+1 and thank you for doing this. Tangential point: github actions are getting more and more useful, they have a public roadmap and now they support using hosted VMs as dedicated workers for a project. If the current set of github actions prove useful and reliable we can consider moving more of our automations/test workloads to github actions.
On Wed, Aug 5, 2020 at 10:00 AM Tyson Hamilton <[email protected]> wrote: > +1 to automating this! > > On Wed, Aug 5, 2020 at 8:38 AM Alexey Romanenko <[email protected]> > wrote: > >> Thanks Robert and Udi for links. Perhaps we should integrate such check >> if there are no objections from community. I’ll take a look on this more >> closely. >> >> On 4 Aug 2020, at 19:31, Udi Meiri <[email protected]> wrote: >> >> https://github.com/marketplace/actions/gs-commit-message-checker >> >> On Tue, Aug 4, 2020 at 10:25 AM Robert Bradshaw <[email protected]> >> wrote: >> >>> +1, thanks for the reminder. >>> >>> This should be really easy to automate, using >>> https://developer.github.com/webhooks/event-payloads/#pull_request to >>> give a warning when the change history is not sufficiently "clean." >>> I'm not sure where to host this though (or if it could be integrated >>> into jenkins--basically I'd just want to run a Python script with the >>> PR number (or better, just point to the local git repo and have the >>> master's commit handy) as another precommit). >>> >>> On Tue, Aug 4, 2020 at 10:10 AM Rui Wang <[email protected]> wrote: >>> > >>> > +1 thanks Alexey. >>> > >>> > My apologies that I merged such a case recently (but not >>> intentionally). I tried to use the "squash and merge" button with a >>> consolidated commit message. After clicking the button, github showed >>> "failed to merge" and gave a retry button, and after clicking that retry >>> button, github magically switched to "create merge commit" approach thus >>> merged some fixup commits to the main branch. >>> > >>> > This is a rare case (I only encountered once). But I will pay more >>> attention next time. I could ask PR authors to squash their commits before >>> merging when it is possible. >>> > >>> > >>> > -Rui >>> > >>> > On Tue, Aug 4, 2020 at 9:40 AM Alexey Romanenko < >>> [email protected]> wrote: >>> >> >>> >> Yes, good point, thanks Valentyn. >>> >> >>> >> On 4 Aug 2020, at 18:29, Valentyn Tymofieiev <[email protected]> >>> wrote: >>> >> >>> >> +1, thanks, Alexey. >>> >> >>> >> Also a reminder from the contributor guide: do not use the default >>> GitHub commit message for merge commits, which looks like: >>> >> >>> >> Merge pull request #1234 from some_user/transient_branch_name >>> >> >>> >> Instead, add the commit message into the subject line, for example: >>> "Merge pull request #1234: [BEAM-7873] Fix the foo bizzle bazzle". >>> >> >>> >> On Tue, Aug 4, 2020 at 7:13 AM Alexey Romanenko < >>> [email protected]> wrote: >>> >>> >>> >>> Hi all, >>> >>> >>> >>> I’d like to attract your attention regarding our Git commit history >>> and related issue. A while ago I noticed that it started getting not very >>> clear and quite verbose comparing to how it was before. We have quite >>> significant amount of recent commits like “fix”, “address comments”, >>> “typo”, “spotless”, etc. Most of them also doesn’t contain Jira Tag as a >>> prefix and actually is just supplementary commits to “main” and initial >>> commit of PR, added after several PRs review rounds. >>> >>> >>> >>> AFAIR, we already had several discussion in the past about this >>> topic and we agreed that we should avoid such commits in a final merge and >>> have only one (in most cases) or several (if necessary) logical commits >>> that should be atomic and properly explain what they do. >>> >>> >>> >>> Why these “tiny" commits are bad practice? Just several main reasons: >>> >>> - They pollute our git repository history and don’t give any >>> additional and useful further information; >>> >>> - They are not atomic and we can’t easily revert (rollback) this >>> supplementary commit since the state of the build before was likely broken >>> or had incorrect behaviour. So, in this case, the whole set of PRs commits >>> should be reverted which is not convenient and error-prone. It’s also >>> expected that all checks were green before merging a PR (take a part flaky >>> tests). >>> >>> - They are not informative in terms of commit message. So it makes >>> more hard to identify Git annotated code and how the lines of code are >>> related together. >>> >>> >>> >>> Following this, I just want to briefly remind our Committers rules >>> regarding PR merging [1]. >>> >>> Every commit: >>> >>> - should do one thing and reflect it in commit message; >>> >>> - should contain Jira Tag; >>> >>> - all “fixup” and “address comments” type of commits should be >>> squashed by author or committer before merging. >>> >>> >>> >>> Please, pay attention on what is finally committed and merged into >>> our repository and it should help to keep our commit history clear, which >>> will be transferred to saving a time of other developers in the end. >>> >>> >>> >>> [1] >>> https://beam.apache.org/contribute/committer-guide/#finishing-touches >>> >>> >>> >>> Regards, >>> >>> Alexey >>> >> >>> >> >>> >> >>
