Hello Alex, > In my opinion checkstyle rules definitely should be checked as early as possible and by default should fail Ignite build. Well, perhaps I am wrong. Ok.
> But for exceptional cases (which was mentioned by Slava), can we add some > "Run custom build" parameter on TC to be able to exclude "checkstyle" > profile from "~Build Apache Ignite~" configuration? WDYT? This is exactly what I wanted to say. Thank you, Alex! Regards, Slava. вт, 7 июл. 2020 г. в 13:01, Alex Plehanov <[email protected]>: > Guys, > > In my opinion checkstyle rules definitely should be checked as early as > possible and by default should fail Ignite build. > But for exceptional cases (which was mentioned by Slava), can we add some > "Run custom build" parameter on TC to be able to exclude "checkstyle" > profile from "~Build Apache Ignite~" configuration? WDYT? > > вт, 7 июл. 2020 г. в 14:53, Вячеслав Коптилин <[email protected]>: > > > Hi Maxim, > > > > > Why the auto-formatting procedure cannot be used for any PR you'd like > to > > run on TC? > > > Even more, you can remove all the rules from checkstyle XML config and > do > > all you want in the PR branch > > Yes, I can enable auto-formatting, which should be checked anyway, I can > > edit pom.xml every time, etc. > > > > My question is the following: Why can't this check be done in parallel > with > > other tasks? > > > > Yep, I see the argument mentioned by Nikolay - TeamCity capacity (we > should > > re-run all tests even though we rename a variable), but I don't see that > > our TC servers are overwhelmed for now. > > Perhaps, I am wrong and this argument (TC capacity) is more significant > > than it seems to me at first glance. > > > > Folks, please don't get me wrong. I am not against the code-style check > at > > all. I just want to get some freedom for dev branches only, if it is > > possible of course. > > > > Thanks, > > Slava. > > > > вт, 7 июл. 2020 г. в 12:21, Maxim Muzafarov <[email protected]>: > > > > > Slava, > > > > > > Why the auto-formatting procedure cannot be used for any PR you'd like > > > to run on TC? Even more, you can remove all the rules from checkstyle > > > XML config and do all you want in the PR branch. > > > > > > On Tue, 7 Jul 2020 at 12:17, Вячеслав Коптилин < > [email protected] > > > > > > wrote: > > > > > > > > Hi Nikolay, > > > > > > > > > Why do you need to Run All on TC resources for this task? > > > > For instance, it may be a race that cannot be reproduced locally on > my > > > own > > > > hardware. > > > > (By the way, even though if I just want to start Cache1 suite, the > > > > Code-Style check executes anyway). > > > > > > > > > If we don’t want to follow the code style or considered it as a > > «waste > > > of > > > > the time» we should change it. > > > > This is absolutely not what I'm trying to say. I do not think, that > > code > > > > style is useless. I just want to say that this check can be done in > > > > parallel for dev branches, for example. > > > > > > > > Thanks, > > > > S. > > > > > > > > вт, 7 июл. 2020 г. в 11:49, Nikolay Izhikov <[email protected]>: > > > > > > > > > > Let's assume that I want to implement a dirty fix of a bug, > check > > a > > > > > reproducer from the user list, etc. > > > > > > > > > > Why do you need to Run All on TC resources for this task? > > > > > > > > > > > I do not want to waste my time fixing all code style violations. > > > > > > > > > > I assume that you have the Ignite project locally because you edit > > > Ignite > > > > > source code. > > > > > If yes, then IntelliJ IDEA has an automatic code formatting and > does > > > all > > > > > the work for you. > > > > > > > > > > > Does this use-case look reasonable? > > > > > > > > > > Yes. > > > > > But, I still don’t see what is the issue here. > > > > > > > > > > If we don’t want to follow the code style or considered it as a > > «waste > > > of > > > > > the time» we should change it. > > > > > As simple as that. > > > > > > > > > > But, we should force the code style as early as we can. > > > > > I think we should fail maven local build on code style violations. > > > > > > > > > > > 7 июля 2020 г., в 11:28, Вячеслав Коптилин < > > [email protected] > > > > > > > > > написал(а): > > > > > > > > > > > > Nikolay, > > > > > > > > > > > > There is *NO *intention to ignore code style violations and do > > merge > > > PRs > > > > > > into the master branch without fixing them. > > > > > > > > > > > > Let's assume that I want to implement a dirty fix of a bug, > check a > > > > > > reproducer from the user list, etc. > > > > > > And I do not have the intention to merge that into the master > > branch > > > as > > > > > is, > > > > > > however, I do not want to waste my time fixing all code style > > > violations. > > > > > > Does this use-case look reasonable? > > > > > > > > > > > > Thanks, > > > > > > Slava. > > > > > > > > > > > > вт, 7 июл. 2020 г. в 11:07, Nikolay Izhikov <[email protected] > >: > > > > > > > > > > > >> Slava. > > > > > >> > > > > > >> All contributors should follow our code style during their > > > contribution. > > > > > >> For now, we have an automatic PR check that is integrated to the > > > GitHub > > > > > >> interface. > > > > > >> > > > > > >> I don’t see any issue here. > > > > > >> All open-source project that I know, uses the same approach. > > > > > >> > > > > > >> Anyway, If some of the experienced community members is > interested > > > in > > > > > the > > > > > >> particular contribution he or she can help a newcomer with the > > code > > > > > style - > > > > > >> GitHub interface has the edit button even for someone else’s > PR’s > > > > > >> > > > > > >> > > > > > >>> 7 июля 2020 г., в 11:01, Вячеслав Коптилин < > > > [email protected]> > > > > > >> написал(а): > > > > > >>> > > > > > >>> Hello Nikolay, > > > > > >>> > > > > > >>>> Because any code style violations should be fixed before the > > > merge. > > > > > >>>> Therefore after the fix, you must rerun TC. > > > > > >>>> This means that running heavy suites when code style is > violated > > > is a > > > > > >>> waste of the resources. > > > > > >>> This makes sense, however, to be honest, I don't see that our > > team > > > city > > > > > >>> servers are really busy. > > > > > >>> > > > > > >>>> Why do you want to violate code style rules in your PR? > > > > > >>> Please take a look at the original e-mail from Ilya: > > > > > >>>> This means that I have completely lost an option to run tests > > > against > > > > > >>> pull > > > > > >>>> requests by new contributors - they usually compile but will > not > > > pass > > > > > >>>> Checkstyle. That's a blocker. > > > > > >>> > > > > > >>> This issue has also been discussed here: > > > > > >>> > > > > > >> > > > > > > > > > > > http://apache-ignite-developers.2346864.n4.nabble.com/DISCUSSION-Separate-code-sanity-check-and-build-task-td47003.html > > > > > >>> > > > > > >>> Thanks, > > > > > >>> Slava. > > > > > >>> > > > > > >>> > > > > > >>> вт, 7 июл. 2020 г. в 10:47, Nikolay Izhikov < > [email protected] > > >: > > > > > >>> > > > > > >>>> All checks just force rules we agreed on. > > > > > >>>> > > > > > >>>>> Why this check is so important? Why do you think it is more > > > important > > > > > >>>> than all other tasks/tests? > > > > > >>>> > > > > > >>>> Because any code style violations should be fixed before the > > > merge. > > > > > >>>> Therefore after the fix, you must rerun TC. > > > > > >>>> This means that running heavy suites when code style is > violated > > > is a > > > > > >>>> waste of the resources. > > > > > >>>> > > > > > >>>> Why do you want to violate code style rules in your PR? > > > > > >>>> > > > > > >>>> I think instead of hiding style errors we should make our code > > > style > > > > > >>>> comfortable for everyone. > > > > > >>>> Can you, please, write - what part of the code style hurts > you? > > > > > >>>> > > > > > >>>> > > > > > >>>>> 7 июля 2020 г., в 10:39, Вячеслав Коптилин < > > > [email protected] > > > > > > > > > > > >>>> написал(а): > > > > > >>>>> > > > > > >>>>> Hello Maxim, > > > > > >>>>> > > > > > >>>>>> Why do you think that splitting the checkstyle build is > better > > > > > option > > > > > >>>>> than fixing code style issues reporting by the checkstyle > > plugin? > > > > > >>>>> Why do you think that Ilya talks that code style errors > should > > > not be > > > > > >>>> fixed? > > > > > >>>>> > > > > > >>>>> It looks weird to me that we do not even start the tests if > > check > > > > > style > > > > > >>>>> plugin reports violations. > > > > > >>>>> Why can't this check be done in parallel with the tests and > > > reported > > > > > by > > > > > >>>>> mtcga bot? > > > > > >>>>> Why this check is so important? Why do you think it is more > > > important > > > > > >>>> than > > > > > >>>>> all other tasks/tests? > > > > > >>>>> > > > > > >>>>> Thanks, > > > > > >>>>> Slava. > > > > > >>>>> > > > > > >>>>> пн, 6 июл. 2020 г. в 20:34, Maxim Muzafarov < > [email protected] > > >: > > > > > >>>>> > > > > > >>>>>> Hello Ilya, > > > > > >>>>>> > > > > > >>>>>> Why do you think that splitting the checkstyle build is > better > > > > > option > > > > > >>>>>> than fixing code style issues reporting by the checkstyle > > > plugin? > > > > > >>>>>> > > > > > >>>>>> On Mon, 6 Jul 2020 at 19:43, Ilya Kasnacheev < > > > > > >> [email protected] > > > > > >>>>> > > > > > >>>>>> wrote: > > > > > >>>>>>> > > > > > >>>>>>> Hello! > > > > > >>>>>>> > > > > > >>>>>>> I have just noticed today that Checkstyle will fail Apache > > > Ignite > > > > > >>>> build: > > > > > >>>>>>> > > > > > >>>>>> > > > > > >>>> > > > > > >> > > > > > > > > > > > https://ci.ignite.apache.org/buildConfiguration/IgniteTests24Java8_BuildApacheIgnite/5443282?buildTab=log&focusLine=3&linesState=683.4289 > > > > > >>>>>>> > > > > > >>>>>>> This means that I have completely lost an option to run > tests > > > > > against > > > > > >>>>>> pull > > > > > >>>>>>> requests by new contributors - they usually compile but > will > > > not > > > > > pass > > > > > >>>>>>> Checkstyle. That's a blocker. > > > > > >>>>>>> > > > > > >>>>>>> Can we please split Checkstyle as a separate build which is > > > > > triggered > > > > > >>>>>> with > > > > > >>>>>>> Run All? > > > > > >>>>>>> I think we even have > > > > > >>>>>>> > > > > > >>>>>> > > > > > >>>> > > > > > >> > > > > > > > > > > > https://ci.ignite.apache.org/buildConfiguration/IgniteTests24Java8_CheckCodeStyle?mode=builds#all-projects > > > > > >>>>>>> > > > > > >>>>>>> WDYT? > > > > > >>>>>>> > > > > > >>>>>>> Regards, > > > > > >>>>>>> -- > > > > > >>>>>>> Ilya Kasnacheev > > > > > >>>>>> > > > > > >>>> > > > > > >>>> > > > > > >> > > > > > >> > > > > > > > > > > > > > > > >
