Re: Checkstyle fails Build Apache Ignite - can we split it?
Ilya, Perhaps I expressed my thoughts vaguely. I have nothing against discussion. I suggest to pin and describe a decision if/when it is made. 2020-07-07 17:10 GMT+03:00, Ilya Kasnacheev : > Hello! > > Ivan, unfortunately, I can't see any formal decision being taken. All I see > in the referenced thread is people who are unhappy with obligatory code > style checking as a prerequisite to running tests. > > Did we hold a formal vote on this issue? Did we even hold an informal vote? > It may turn up that it was a voluntary decision by a sole fellow > committer which deserves not pinning but discussion. > > Regards, > -- > Ilya Kasnacheev > > > вт, 7 июл. 2020 г. в 11:50, Ivan Pavlukhin : > >> Folks, >> >> This discussion reoccurs from month to month. Last one I remember is >> [1]. It sounds useful to pin a decision to our coding guidelines. What >> do you think? >> >> [1] >> http://apache-ignite-developers.2346864.n4.nabble.com/DISCUSSION-Separate-code-sanity-check-and-build-task-td47003.html >> >> 2020-07-07 11:28 GMT+03:00, Вячеслав Коптилин : >> > 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 : >> > >> >> 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, Вячеслав Коптилин >> >> > >> >> написал(а): >> >> > >> >> > 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 : >> >> > >> >> >> 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, Вячеслав Коптилин < >> slava.kopti...@gmail.com> >> >> >> написал(а): >> >> >>> >> >> >>> 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,
Re: Checkstyle fails Build Apache Ignite - can we split it?
Hello! Ivan, unfortunately, I can't see any formal decision being taken. All I see in the referenced thread is people who are unhappy with obligatory code style checking as a prerequisite to running tests. Did we hold a formal vote on this issue? Did we even hold an informal vote? It may turn up that it was a voluntary decision by a sole fellow committer which deserves not pinning but discussion. Regards, -- Ilya Kasnacheev вт, 7 июл. 2020 г. в 11:50, Ivan Pavlukhin : > Folks, > > This discussion reoccurs from month to month. Last one I remember is > [1]. It sounds useful to pin a decision to our coding guidelines. What > do you think? > > [1] > http://apache-ignite-developers.2346864.n4.nabble.com/DISCUSSION-Separate-code-sanity-check-and-build-task-td47003.html > > 2020-07-07 11:28 GMT+03:00, Вячеслав Коптилин : > > 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 : > > > >> 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, Вячеслав Коптилин > >> написал(а): > >> > > >> > 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 : > >> > > >> >> 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, Вячеслав Коптилин < > slava.kopti...@gmail.com> > >> >> написал(а): > >> >>> > >> >>> 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 : > >> >>> > >> 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 < > >> ilya.kasnach...@gmail.com > >> >>> > >> wrote: > >> > > >> > Hello! > >> > > >> > I have just noticed
Re: Checkstyle fails Build Apache Ignite - can we split it?
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 : > 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, Вячеслав Коптилин : > > > 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 : > > > > > 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, Вячеслав Коптилин < > slava.kopti...@gmail.com > > > > > > 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 : > > > > > > > > > > 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, Вячеслав Коптилин < > > slava.kopti...@gmail.com > > > > > > > > > написал(а): > > > > > > > > > > > > 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 >: > > > > > > > > > > > >> Slava. > > > > > >> > > > > > >> All contributors should follow our code style
Re: Checkstyle fails Build Apache Ignite - can we split it?
> > I am not against the code-style check at all. > Yes, you are :) But it’s ok No, I am not :) > You can take a look at the commits that introduced checks and how many files were touched. I really appreciate this, Nikolay. Thanks, S. вт, 7 июл. 2020 г. в 13:04, Nikolay Izhikov : > > Why can't this check be done in parallel with other tasks? > > Because when we contribute code to Ignite we must follow code style. > > > I am not against the code-style check at all. > > Yes, you are :) But it’s ok > > > I just want to get some freedom for dev branches only, if it is possible > of course. > > It’s not about freedom - you are free to run any checks you want on TC. > It’s about project codebase consistency. > > No long time ago we don’t have TC bot or checkstyle plugin. > That leads us to new and new tests failures and code style violations. > > You can take a look at the commits that introduced checks and how many > files were touched. > We just ignore our own rules without checks. > > > https://github.com/apache/ignite/commit/2fbbb676386515ea881e4e61f08864d6bc93225a > > https://github.com/apache/ignite/commit/2a85925f1705fbad36b5421c0ca5cf9de9a29658 > > https://github.com/apache/ignite/commit/d63f4d3569dcb387394d367a7f00aaf35f1b288e > > > > > 7 июля 2020 г., в 12:52, Вячеслав Коптилин > написал(а): > > > > 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 : > > > >> 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, Вячеслав Коптилин < > slava.kopti...@gmail.com> > >> 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 : > >>> > > 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, Вячеслав Коптилин >>> > написал(а): > > > > 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. > > >
Re: Checkstyle fails Build Apache Ignite - can we split it?
> Why can't this check be done in parallel with other tasks? Because when we contribute code to Ignite we must follow code style. > I am not against the code-style check at all. Yes, you are :) But it’s ok > I just want to get some freedom for dev branches only, if it is possible of > course. It’s not about freedom - you are free to run any checks you want on TC. It’s about project codebase consistency. No long time ago we don’t have TC bot or checkstyle plugin. That leads us to new and new tests failures and code style violations. You can take a look at the commits that introduced checks and how many files were touched. We just ignore our own rules without checks. https://github.com/apache/ignite/commit/2fbbb676386515ea881e4e61f08864d6bc93225a https://github.com/apache/ignite/commit/2a85925f1705fbad36b5421c0ca5cf9de9a29658 https://github.com/apache/ignite/commit/d63f4d3569dcb387394d367a7f00aaf35f1b288e > 7 июля 2020 г., в 12:52, Вячеслав Коптилин > написал(а): > > 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 : > >> 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, Вячеслав Коптилин >> 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 : >>> > 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, Вячеслав Коптилин >> написал(а): > > 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 : > >> 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 -
Re: Checkstyle fails Build Apache Ignite - can we split it?
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, Вячеслав Коптилин : > 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 : > > > 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, Вячеслав Коптилин > > > 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 : > > > > > > > > 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, Вячеслав Коптилин < > slava.kopti...@gmail.com > > > > > > > написал(а): > > > > > > > > > > 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 : > > > > > > > > > >> 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, Вячеслав Коптилин < > > slava.kopti...@gmail.com> > > > > >> написал(а): > > > > >>> > > > > >>> Hello Nikolay, > > > > >>> > > > > Because any code style violations should be fixed before the >
Re: Checkstyle fails Build Apache Ignite - can we split it?
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 : > 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, Вячеслав Коптилин > 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 : > > > > > > 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, Вячеслав Коптилин > > > > написал(а): > > > > > > > > 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 : > > > > > > > >> 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, Вячеслав Коптилин < > slava.kopti...@gmail.com> > > > >> написал(а): > > > >>> > > > >>> 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
Re: Checkstyle fails Build Apache Ignite - can we split it?
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, Вячеслав Коптилин 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 : > > > > 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, Вячеслав Коптилин > > написал(а): > > > > > > 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 : > > > > > >> 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, Вячеслав Коптилин > > >> написал(а): > > >>> > > >>> 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 : > > >>> > > 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, Вячеслав Коптилин > > > > написал(а): > > > > > > Hello Maxim, > > > > > >> Why do you think that splitting the checkstyle build is better >
Re: Checkstyle fails Build Apache Ignite - can we split it?
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 : > > 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, Вячеслав Коптилин > написал(а): > > > > 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 : > > > >> 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, Вячеслав Коптилин > >> написал(а): > >>> > >>> 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 : > >>> > 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, Вячеслав Коптилин > > написал(а): > > > > 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
Re: Checkstyle fails Build Apache Ignite - can we split it?
Folks, In addition to answers above, I think TC.Bot must comment PR with violated checkstyle rules and references to source code if the build has failed. This may be more convenient for contributors from my point of view. On Tue, 7 Jul 2020 at 11:50, Ivan Pavlukhin wrote: > > Folks, > > This discussion reoccurs from month to month. Last one I remember is > [1]. It sounds useful to pin a decision to our coding guidelines. What > do you think? > > [1] > http://apache-ignite-developers.2346864.n4.nabble.com/DISCUSSION-Separate-code-sanity-check-and-build-task-td47003.html > > 2020-07-07 11:28 GMT+03:00, Вячеслав Коптилин : > > 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 : > > > >> 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, Вячеслав Коптилин > >> написал(а): > >> > > >> > 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 : > >> > > >> >> 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, Вячеслав Коптилин > >> >> написал(а): > >> >>> > >> >>> 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 : > >> >>> > >> 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 < > >> ilya.kasnach...@gmail.com > >> >>> > >> 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=3=683.4289 > >> > > >> > This means that
Re: Checkstyle fails Build Apache Ignite - can we split it?
Folks, This discussion reoccurs from month to month. Last one I remember is [1]. It sounds useful to pin a decision to our coding guidelines. What do you think? [1] http://apache-ignite-developers.2346864.n4.nabble.com/DISCUSSION-Separate-code-sanity-check-and-build-task-td47003.html 2020-07-07 11:28 GMT+03:00, Вячеслав Коптилин : > 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 : > >> 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, Вячеслав Коптилин >> написал(а): >> > >> > 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 : >> > >> >> 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, Вячеслав Коптилин >> >> написал(а): >> >>> >> >>> 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 : >> >>> >> 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 < >> ilya.kasnach...@gmail.com >> >>> >> 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=3=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? >> > >> >
Re: Checkstyle fails Build Apache Ignite - can we split it?
> 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, Вячеслав Коптилин > написал(а): > > 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 : > >> 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, Вячеслав Коптилин >> написал(а): >>> >>> 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 : >>> 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, Вячеслав Коптилин написал(а): > > 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 : > >> 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 < >> ilya.kasnach...@gmail.com > >> 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=3=683.4289 >>> >>> This means that I have completely lost an option to run tests against >> pull >>> requests by new contributors - they usually compile but
Re: Checkstyle fails Build Apache Ignite - can we split it?
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 : > 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, Вячеслав Коптилин > написал(а): > > > > 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 : > > > >> 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, Вячеслав Коптилин > >> написал(а): > >>> > >>> 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 : > >>> > 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 < > ilya.kasnach...@gmail.com > >>> > 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=3=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 > > >> > >> > >
Re: Checkstyle fails Build Apache Ignite - can we split it?
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, Вячеслав Коптилин > написал(а): > > 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 : > >> 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, Вячеслав Коптилин >> написал(а): >>> >>> 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 : >>> 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 >> 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=3=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 >> >>
Re: Checkstyle fails Build Apache Ignite - can we split it?
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 : > 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, Вячеслав Коптилин > написал(а): > > > > 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 : > > > >> 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 > > >> 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=3=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 > >> > >
Re: Checkstyle fails Build Apache Ignite - can we split it?
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, Вячеслав Коптилин > написал(а): > > 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 : > >> 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 >> 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=3=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 >>
Re: Checkstyle fails Build Apache Ignite - can we split it?
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 : > 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 > 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=3=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 >
Re: Checkstyle fails Build Apache Ignite - can we split it?
I think, that we should enable checkstyle profile by default. Checkstyle should run even before compile lifecycle, may be on process-source. Codestyle problem should be handled immediately, but not on quite expensive TC run. пн, 6 июл. 2020 г. в 20:34, Maxim Muzafarov : > 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 > 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=3=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 > -- Sincerely yours, Ivan Daschinskiy
Re: Checkstyle fails Build Apache Ignite - can we split it?
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 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=3=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
Checkstyle fails Build Apache Ignite - can we split it?
Hello! I have just noticed today that Checkstyle will fail Apache Ignite build: https://ci.ignite.apache.org/buildConfiguration/IgniteTests24Java8_BuildApacheIgnite/5443282?buildTab=log=3=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
