ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Pavel Tupitsyn <ptupit...@apache.org>
Subject Re: [DISCUSSION] Pull-request checks on GitHub
Date Sun, 03 May 2020 10:07:47 GMT
Igniters, Maxim,

I think this thread is a good opportunity to discuss commit message
guidelines.
I suggest the following:

1. Treat PR title + description as the final squashed commit message.
PR author is responsible for writing that properly.
Committer who merges the PR is responsible for validating that and using
that for the actual squash commit.

2. Adopt the following Git commit message rules (partially from
https://chris.beams.io/posts/git-commit/):
- Start with IGNITE-NNNN
- Use imperative mood in the subject line ("Fix foobar crash on start",
"Add baz metric")
- Capitalize the subject line
- Do not end the subject line with a period
- Use the body to explain what and why vs. how

Thoughts?

On Sun, May 3, 2020 at 11:53 AM Maxim Muzafarov <mmuzaf@apache.org> wrote:

> Hello,
>
> I have the following in my mind:
> 1. This checklist is for discussion and may be changed.
> 2. Commits can be squashed in the branch prior to asking a review, but
> when the review is in progress a good naming may help to understand
> the changes.
> 3. It's true that the commit message can be changed prior to merging
> the master branch, but it's better to merge the PR with an initial
> authored commit message `as is`.
>
> On Sat, 2 May 2020 at 18:20, Guru Stron <gurustronpublic@gmail.com> wrote:
> >
> > Maxim,
> >
> > I have a small question about "Commits have the following pattern..". Is
> > it really needed cause AFAIK commits in the PR are squashed. Or am  I
> > missing something?
> >
> > On Thu, Apr 30, 2020, 8:33 PM Maxim Muzafarov <mmuzaf@apache.org> wrote:
> >
> > > Folks,
> > >
> > >
> > > I've created the pull request template for GitHub.
> > > Please, take a look and let me know what you think [1] [2].
> > >
> > >
> > > [1]
> > >
> https://github.com/apache/ignite/pull/7765/files#diff-195a635ad245ded9076330e31134bd80
> > > [2] https://issues.apache.org/jira/browse/IGNITE-12937
> > >
> > > On Sun, 26 Apr 2020 at 20:35, Saikat Maitra <saikat.maitra@gmail.com>
> > > wrote:
> > > >
> > > > Hi Maxim,
> > > >
> > > > Thank you for enabling travis ci in ignite repo. It is very helpful
> to
> > > see
> > > > PR build results integrated in PR request.
> > > >
> > > > I will enable it in ignite-extensions repo as well.
> > > >
> > > > Regards,
> > > > Saikat
> > > >
> > > > On Mon, Apr 20, 2020 at 12:14 PM Pavel Tupitsyn <
> ptupitsyn@apache.org>
> > > > wrote:
> > > >
> > > > > Maxim, pull request template is a great idea.
> > > > > We can put a checklist there along with the links to the
> guidelines,
> > > > > something like this:
> > > > >
> > > > > [ ] Coding Guidelines are followed
> > > > > [ ] TeamCity build passes
> > > > > [ ] JIRA ticked is in Patch Available state, review has been
> requested
> > > in
> > > > > comments
> > > > > [ ] Something else?
> > > > >
> > > > > On Mon, Apr 20, 2020 at 8:09 PM Maxim Muzafarov <mmuzaf@apache.org
> >
> > > wrote:
> > > > >
> > > > > > Pavel,
> > > > > >
> > > > > > Sorry for the incomplete message.
> > > > > >
> > > > > > 2. I mentioned it too. Hopefully, builds > 4 hrs would not
be too
> > > often.
> > > > > > The reason - there are limited job-workers shared between all
the
> > > > > > Apache projects. I found some details here [1] [2].
> > > > > >
> > > > > >
> > > > > > [1]
> > > > > >
> > > > >
> > >
> https://lists.apache.org/thread.html/af52e2a3e865c01596d46374e8b294f2740587dbd59d85e132429b6c@%3Cbuilds.apache.org%3E
> > > > > > [2] https://issues.apache.org/jira/browse/INFRA-18533
> > > > > >
> > > > > > On Mon, 20 Apr 2020 at 20:03, Maxim Muzafarov <mmuzaf@apache.org
> >
> > > wrote:
> > > > > > >
> > > > > > > Pavel,
> > > > > > >
> > > > > > > 1. Agree here. What if we create a default template pull
> request
> > > > > > > description with all the links required by our development
> process?
> > > > > > > [1] It's will be friendly for contributors to have everything
> they
> > > > > > > need in the PR.
> > > > > > >
> > > > > > > 2.
> > > > > > >
> > > > > > > [1]
> > > > > >
> > > > >
> > >
> https://help.github.com/en/github/building-a-strong-community/creating-a-pull-request-template-for-your-repository
> > > > > > >
> > > > > > > On Mon, 20 Apr 2020 at 19:46, Pavel Tupitsyn <
> ptupitsyn@apache.org
> > > >
> > > > > > wrote:
> > > > > > > >
> > > > > > > > Maxim,
> > > > > > > >
> > > > > > > > Good news, thank you.
> > > > > > > >
> > > > > > > > However, I see two issues with this:
> > > > > > > >
> > > > > > > > 1. False sense of a ready-to-merge PR
> > > > > > > > Now that we have a reassuring green checkmark on the
PR,
> > > contributors
> > > > > > might
> > > > > > > > think that build passes and all is well.
> > > > > > > > But this is not true - we only check that the code
compiles.
> > > TeamCity
> > > > > > run
> > > > > > > > is still required.
> > > > > > > > My proposal is to change the text somehow to make
this clear,
> > > maybe
> > > > > > add a
> > > > > > > > link to the contribution guidelines automatically.
> > > > > > > >
> > > > > > > > 2. Builds seem to spend a lot of time in the queue.
> > > > > > > > I've created this PR 4 hours ago, still no results:
[1]
> > > > > > > > Any ideas? I use Travis on some other GitHub projects
and it
> > > usually
> > > > > > runs
> > > > > > > > in a minute or two.
> > > > > > > >
> > > > > > > > [1] https://github.com/apache/ignite/pull/7698
> > > > > > > >
> > > > > > > > On Mon, Apr 20, 2020 at 3:16 PM Maxim Muzafarov <
> > > mmuzaf@apache.org>
> > > > > > wrote:
> > > > > > > >
> > > > > > > > > Igniters,
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > The Travis-ci build configured for running on
the Apache
> > > Ignite PRs
> > > > > > > > > and the master branch [1] [2].
> > > > > > > > >
> > > > > > > > > Build run under:
> > > > > > > > > openjdk8
> > > > > > > > > openjdk11
> > > > > > > > >
> > > > > > > > > Example of PR:
> > > > > > > > > https://github.com/apache/ignite/pull/7695
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > [1] https://travis-ci.org/github/apache/ignite
> > > > > > > > > [2] https://issues.apache.org/jira/browse/IGNITE-12916
> > > > > > > > >
> > > > > > > > > On Tue, 14 Apr 2020 at 21:00, Maxim Muzafarov
<
> > > mmuzaf@apache.org>
> > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > Petr,
> > > > > > > > > >
> > > > > > > > > > I think it's doable. It has custom `install-jdk`
script,
> so
> > > even
> > > > > > the
> > > > > > > > > > latest JDKs can be used.
> > > > > > > > > >
> > > > > > > > > > [1] https://github.com/sormuras/bach#install-jdksh
> > > > > > > > > >
> > > > > > > > > > On Tue, 14 Apr 2020 at 18:31, Petr Ivanov
<
> > > mr.weider@gmail.com>
> > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > We do not need JDK10 — it is out
of support already.
> > > > > > > > > > > Instead, how about adding JDK14?
> > > > > > > > > > >
> > > > > > > > > > > > On 14 Apr 2020, at 17:30, Maxim
Muzafarov <
> > > mmuzaf@apache.org
> > > > > >
> > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Folks,
> > > > > > > > > > > >
> > > > > > > > > > > > I forgot to mention one more important
thing of this
> > > tool. We
> > > > > > can
> > > > > > > > > > > > configure build and checks simultaneously
for
> several JDK
> > > > > > versions.
> > > > > > > > > > > >
> > > > > > > > > > > > jdk:
> > > > > > > > > > > >  - oraclejdk8
> > > > > > > > > > > >  - openjdk10
> > > > > > > > > > > >  - openjdk11
> > > > > > > > > > > >
> > > > > > > > > > > > On Tue, 14 Apr 2020 at 17:17,
Maxim Muzafarov <
> > > > > > mmuzaf@apache.org>
> > > > > > > > > wrote:
> > > > > > > > > > > >>
> > > > > > > > > > > >> Folks,
> > > > > > > > > > > >>
> > > > > > > > > > > >> +1 Travis-ci
> > > > > > > > > > > >>
> > > > > > > > > > > >> I see no disadvantages of
having multiple CI tools
> due
> > > to:
> > > > > > > > > > > >> - it's free for open-source
and can be disabled at
> any
> > > time
> > > > > > without
> > > > > > > > > > > >> any consequences;
> > > > > > > > > > > >> - it will free TeamCity from
running builds on each
> PR
> > > and
> > > > > TC
> > > > > > can
> > > > > > > > > > > >> focus on tests execution;
> > > > > > > > > > > >> - we can perform more sophisticated
checks with this
> > > tool
> > > > > > like a PR
> > > > > > > > > > > >> title format (e.g. IGNITE-XXXXX:
Sample)
> > > > > > > > > > > >>
> > > > > > > > > > > >> It seems the TC.Bot can also
be integrated with
> GitHub
> > > > > checks
> > > > > > via
> > > > > > > > > REST API [1].
> > > > > > > > > > > >>
> > > > > > > > > > > >>
> > > > > > > > > > > >> I've checked locally the Ignite
build procedure with
> > > > > > travis-ci and
> > > > > > > > > > > >> GitHub checks [2] and looks
like everything is
> working
> > > fine.
> > > > > > > > > > > >> Who can configure the similar
things on Apache
> Ignite
> > > GitHub
> > > > > > mirror?
> > > > > > > > > > > >> Does anyone have such access
rights?
> > > > > > > > > > > >>
> > > > > > > > > > > >>
> > > > > > > > > > > >> [1] https://developer.github.com/v3/checks/runs/
> > > > > > > > > > > >> [2]
> > > > > > > > >
> > > > > >
> > > https://github.com/Mmuzaf/ignite/pull/1/checks?check_run_id=584537955
> > > > > > > > > > > >>
> > > > > > > > > > > >> On Tue, 14 Apr 2020 at 10:37,
Nikolay Izhikov <
> > > > > > nizhikov@apache.org>
> > > > > > > > > wrote:
> > > > > > > > > > > >>>
> > > > > > > > > > > >>>> On another hand, it
seems weird to have both
> TeamCity
> > > and
> > > > > > Travis
> > > > > > > > > > > >>>
> > > > > > > > > > > >>> And don’t forget MTCGA
bot!
> > > > > > > > > > > >>>
> > > > > > > > > > > >>>
> > > > > > > > > > > >>>> 14 апр. 2020 г.,
в 10:23, Pavel Tupitsyn <
> > > > > > ptupitsyn@apache.org>
> > > > > > > > > написал(а):
> > > > > > > > > > > >>>>
> > > > > > > > > > > >>>> We should have PR
checks for sure.
> > > > > > > > > > > >>>>
> > > > > > > > > > > >>>> On one hand, I agree
with Denis:
> > > > > > > > > > > >>>> - Travis is very easy
to set up in GitHub
> > > > > > > > > > > >>>> - Config file (travis.yml)
is stored in git,
> which is
> > > > > great
> > > > > > > > > > > >>>>
> > > > > > > > > > > >>>> On another hand, it
seems weird to have both
> TeamCity
> > > and
> > > > > > Travis.
> > > > > > > > > > > >>>> Thoughts?
> > > > > > > > > > > >>>>
> > > > > > > > > > > >>>> On Tue, Apr 14, 2020
at 10:16 AM Denis Garus <
> > > > > > garus.d.g@gmail.com>
> > > > > > > > > wrote:
> > > > > > > > > > > >>>>
> > > > > > > > > > > >>>>> Hello!
> > > > > > > > > > > >>>>>
> > > > > > > > > > > >>>>> I have seen projects
with Travis-ci they look
> cool.
> > > > > > > > > > > >>>>> I think Travis-ci
is a good solution.
> > > > > > > > > > > >>>>>
> > > > > > > > > > > >>>>> вт, 14 апр.
2020 г. в 10:00, Andrey Mashenkov <
> > > > > > > > > andrey.mashenkov@gmail.com
> > > > > > > > > > > >>>>>> :
> > > > > > > > > > > >>>>>
> > > > > > > > > > > >>>>>> Maxim,
> > > > > > > > > > > >>>>>>
> > > > > > > > > > > >>>>>> Good idea.
I'd add a license check as well.
> > > > > > > > > > > >>>>>>
> > > > > > > > > > > >>>>>> On Tue, Apr
14, 2020 at 2:14 AM Maxim Muzafarov
> <
> > > > > > > > > mmuzaf@apache.org>
> > > > > > > > > > > >>>>> wrote:
> > > > > > > > > > > >>>>>>
> > > > > > > > > > > >>>>>>> Igniters,
> > > > > > > > > > > >>>>>>>
> > > > > > > > > > > >>>>>>> It's really
`must-have` feature for me to
> enable
> > > Apache
> > > > > > Ignite
> > > > > > > > > > > >>>>>>> pull-request
status checks on GitHub.
> Currently we
> > > > > don't
> > > > > > have
> > > > > > > > > any of
> > > > > > > > > > > >>>>>>> them.
The most obvious check for each
> pull-request
> > > is:
> > > > > > > > > > > >>>>>>> - build
the source code and check code-style
> > > > > violations;
> > > > > > > > > > > >>>>>>>
> > > > > > > > > > > >>>>>>> This will
give us some advantages. For
> instance:
> > > > > > > > > > > >>>>>>> 1. Each
PR even a very simple (not require
> tests
> > > > > > execution)
> > > > > > > > > will be
> > > > > > > > > > > >>>>>>> checked
by checkstyle and for compile errors.
> > > > > > > > > > > >>>>>>> 2. Developers
can be get notified earlier if
> > > checkstyle
> > > > > > has
> > > > > > > > > been
> > > > > > > > > > > >>>>>>> violated
in their PRs.
> > > > > > > > > > > >>>>>>>
> > > > > > > > > > > >>>>>>> To achieve
this we can do the following:
> > > > > > > > > > > >>>>>>> 1. Configure
our TeamCity integration with the
> > > Ignite
> > > > > > GitHub
> > > > > > > > > > > >>>>>>> repository
and PR build. It seems it is
> possible
> > > [2].
> > > > > > > > > > > >>>>>>> 2. Use
Travis-ci which is free for open-source
> > > projects
> > > > > > and
> > > > > > > > > also has
> > > > > > > > > > > >>>>>>> an integration
with GitHub checks [1].
> > > > > > > > > > > >>>>>>>
> > > > > > > > > > > >>>>>>>
> > > > > > > > > > > >>>>>>> What do
you think?
> > > > > > > > > > > >>>>>>> What options
will be the best for us?
> > > > > > > > > > > >>>>>>>
> > > > > > > > > > > >>>>>>> [1]
> > > > > > > > > > > >>>>>>>
> > > > > > > > > > > >>>>>>
> > > > > > > > > > > >>>>>
> > > > > > > > >
> > > > > >
> > > > >
> > >
> https://blog.travis-ci.com/2018-05-07-announcing-support-for-github-checks-api-on-travis-ci-com
> > > > > > > > > > > >>>>>>> [2]
> > > > > > > > > > > >>>>>>>
> > > > > > > > > > > >>>>>>
> > > > > > > > > > > >>>>>
> > > > > > > > >
> > > > > >
> > > > >
> > >
> https://himynameistim.com/2018/01/16/adding-build-statuses-to-pull-requests-with-teamcity-and-github/
> > > > > > > > > > > >>>>>>>
> > > > > > > > > > > >>>>>>
> > > > > > > > > > > >>>>>>
> > > > > > > > > > > >>>>>> --
> > > > > > > > > > > >>>>>> Best regards,
> > > > > > > > > > > >>>>>> Andrey V.
Mashenkov
> > > > > > > > > > > >>>>>>
> > > > > > > > > > > >>>>>
> > > > > > > > > > > >>>
> > > > > > > > > > >
> > > > > > > > >
> > > > > >
> > > > >
> > >
> > Y
> >
> > >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message