You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flink.apache.org by Tom Cooper <co...@tomcooper.dev> on 2025/03/21 16:57:38 UTC
CHI: Add liiniting checks to all PRs
Hey All,
I wanted to start a discussion on enabling linting checks, via a GitHub Action, on all PRs in the main Flink repository.
Often when a user submits a PR they will wait for the CI to run in Azure DevOps and this will fail as they have forgotten to run spotless or checkstyle. However, in order to find this out, the author has to dig through the Azure DevOps UI and often parse the logs.
We currently have a GitHub Action for running "pre-compile" checks that include all the standard linting checks: .github/workflows/template.pre-compile-checks.yml [1]. However this is only run as part of the nightly (beta) runs.
We have discussed, in the CHI meetings, how it would be a better experience for developers if this was run for every PR (using the default JVM, currently 17). That way a clear indication would be given, via the GitHub UI (green ticks and red crosses), if the PR has passed the basic checks or not.
This would hopefully speed up the review process.
I have raised a JIRA for this [2] and, initially, this would probably just consist of calling the pre-compile checks workflow as is from a top level PR check workflow. We could then add any further checks we identify in the future to that workflow.
Let me know what you think?
Cheers,
Tom Cooper
@tomcooper.dev | https://tomcooper.dev
[1] https://github.com/apache/flink/blob/master/.github/workflows/template.pre-compile-checks.yml
[2] https://issues.apache.org/jira/browse/FLINK-37534
Re: CHI: Add liiniting checks to all PRs
Posted by Tom Cooper <co...@tomcooper.dev>.
I agree a pre-commit hook for these linting checks, should be available and recommended in the developer documentation (I will put together a PR for this).
However, you can't enforce pre-commit hooks AFAIK so having something running in CI would still help.
Tom Cooper
@tomcooper.dev | https://tomcooper.dev
On Saturday, 29 March 2025 at 02:38, Neelesh Salian <ne...@gmail.com> wrote:
> Hi Tom,
>
> +1 to the idea. I would suggest pre-commit hooks be a good alternative as
> they allow the developer to have more confidence before pushing the PR and
> waiting for CI/CD to pick it up. Thoughts?
>
> Regards,
> Neelesh S. Salian
>
>
>
> On Sat, Mar 29, 2025 at 00:19 Ferenc Csaky ferenc.csaky@pm.me.invalid
>
> wrote:
>
> > Hi Tom,
> >
> > Big +1 for this. On a slightly different note, do we know why the
> > CI run is still in beta? I see that it currently fails because of
> > Python problems, but AFAIK it was not really touched for quite some
> > time.
> >
> > Is it just needs some more development time, or we have resource
> > limitations, maybe some ASF requirements to finalize its adoption
> > and make it "production ready"? IMO it would be a good thing to
> > wrap up this whole story. Furthermore, it runs for every commit, so
> > we also use twice as much resources for pretty much nothing at the
> > moment.
> >
> > WDYT?
> >
> > Best,
> > Ferenc
> >
> > On Friday, March 28th, 2025 at 15:21, Arvid Heise
> > aheise@confluent.io.INVALID wrote:
> >
> > > +1, this will save time for everyone.
> > >
> > > On Fri, Mar 21, 2025 at 5:58 PM Tom Cooper code@tomcooper.dev wrote:
> > >
> > > > Hey All,
> > > >
> > > > I wanted to start a discussion on enabling linting checks, via a
> > > > GitHub Action, on all PRs in the main Flink repository.
> > > >
> > > > Often when a user submits a PR they will wait for the CI to run in
> > > > Azure DevOps and this will fail as they have forgotten to run spotless or
> > > > checkstyle. However, in order to find this out, the author has to dig
> > > > through the Azure DevOps UI and often parse the logs.
> > > >
> > > > We currently have a GitHub Action for running "pre-compile" checks
> > > > that include all the standard linting checks:
> > > > .github/workflows/template.pre-compile-checks.yml [1]. However this is only
> > > > run as part of the nightly (beta) runs.
> > > >
> > > > We have discussed, in the CHI meetings, how it would be a better
> > > > experience for developers if this was run for every PR (using the default
> > > > JVM, currently 17). That way a clear indication would be given, via the
> > > > GitHub UI (green ticks and red crosses), if the PR has passed the basic
> > > > checks or not.
> > > >
> > > > This would hopefully speed up the review process.
> > > >
> > > > I have raised a JIRA for this [2] and, initially, this would probably
> > > > just consist of calling the pre-compile checks workflow as is from a top
> > > > level PR check workflow. We could then add any further checks we identify
> > > > in the future to that workflow.
> > > >
> > > > Let me know what you think?
> > > >
> > > > Cheers,
> > > >
> > > > Tom Cooper
> > > > @tomcooper.dev | https://tomcooper.dev
> > > >
> > > > [1]
> > > > https://github.com/apache/flink/blob/master/.github/workflows/template.pre-compile-checks.yml
> > > > [2] https://issues.apache.org/jira/browse/FLINK-37534
Re: CHI: Add liiniting checks to all PRs
Posted by Neelesh Salian <ne...@gmail.com>.
Hi Tom,
+1 to the idea. I would suggest pre-commit hooks be a good alternative as
they allow the developer to have more confidence before pushing the PR and
waiting for CI/CD to pick it up. Thoughts?
Regards,
Neelesh S. Salian
On Sat, Mar 29, 2025 at 00:19 Ferenc Csaky <fe...@pm.me.invalid>
wrote:
> Hi Tom,
>
> Big +1 for this. On a slightly different note, do we know why the
> CI run is still in beta? I see that it currently fails because of
> Python problems, but AFAIK it was not really touched for quite some
> time.
>
> Is it just needs some more development time, or we have resource
> limitations, maybe some ASF requirements to finalize its adoption
> and make it "production ready"? IMO it would be a good thing to
> wrap up this whole story. Furthermore, it runs for every commit, so
> we also use twice as much resources for pretty much nothing at the
> moment.
>
> WDYT?
>
> Best,
> Ferenc
>
> On Friday, March 28th, 2025 at 15:21, Arvid Heise
> <ah...@confluent.io.INVALID> wrote:
>
> >
> >
> > +1, this will save time for everyone.
> >
> > On Fri, Mar 21, 2025 at 5:58 PM Tom Cooper code@tomcooper.dev wrote:
> >
> > > Hey All,
> > >
> > > I wanted to start a discussion on enabling linting checks, via a
> GitHub Action, on all PRs in the main Flink repository.
> > >
> > > Often when a user submits a PR they will wait for the CI to run in
> Azure DevOps and this will fail as they have forgotten to run spotless or
> checkstyle. However, in order to find this out, the author has to dig
> through the Azure DevOps UI and often parse the logs.
> > >
> > > We currently have a GitHub Action for running "pre-compile" checks
> that include all the standard linting checks:
> .github/workflows/template.pre-compile-checks.yml [1]. However this is only
> run as part of the nightly (beta) runs.
> > >
> > > We have discussed, in the CHI meetings, how it would be a better
> experience for developers if this was run for every PR (using the default
> JVM, currently 17). That way a clear indication would be given, via the
> GitHub UI (green ticks and red crosses), if the PR has passed the basic
> checks or not.
> > >
> > > This would hopefully speed up the review process.
> > >
> > > I have raised a JIRA for this [2] and, initially, this would probably
> just consist of calling the pre-compile checks workflow as is from a top
> level PR check workflow. We could then add any further checks we identify
> in the future to that workflow.
> > >
> > > Let me know what you think?
> > >
> > > Cheers,
> > >
> > > Tom Cooper
> > > @tomcooper.dev | https://tomcooper.dev
> > >
> > > [1]
> https://github.com/apache/flink/blob/master/.github/workflows/template.pre-compile-checks.yml
> > > [2] https://issues.apache.org/jira/browse/FLINK-37534
>
Re: CHI: Add liiniting checks to all PRs
Posted by Ferenc Csaky <fe...@pm.me.INVALID>.
Hi Tom,
Big +1 for this. On a slightly different note, do we know why the
CI run is still in beta? I see that it currently fails because of
Python problems, but AFAIK it was not really touched for quite some
time.
Is it just needs some more development time, or we have resource
limitations, maybe some ASF requirements to finalize its adoption
and make it "production ready"? IMO it would be a good thing to
wrap up this whole story. Furthermore, it runs for every commit, so
we also use twice as much resources for pretty much nothing at the
moment.
WDYT?
Best,
Ferenc
On Friday, March 28th, 2025 at 15:21, Arvid Heise <ah...@confluent.io.INVALID> wrote:
>
>
> +1, this will save time for everyone.
>
> On Fri, Mar 21, 2025 at 5:58 PM Tom Cooper code@tomcooper.dev wrote:
>
> > Hey All,
> >
> > I wanted to start a discussion on enabling linting checks, via a GitHub Action, on all PRs in the main Flink repository.
> >
> > Often when a user submits a PR they will wait for the CI to run in Azure DevOps and this will fail as they have forgotten to run spotless or checkstyle. However, in order to find this out, the author has to dig through the Azure DevOps UI and often parse the logs.
> >
> > We currently have a GitHub Action for running "pre-compile" checks that include all the standard linting checks: .github/workflows/template.pre-compile-checks.yml [1]. However this is only run as part of the nightly (beta) runs.
> >
> > We have discussed, in the CHI meetings, how it would be a better experience for developers if this was run for every PR (using the default JVM, currently 17). That way a clear indication would be given, via the GitHub UI (green ticks and red crosses), if the PR has passed the basic checks or not.
> >
> > This would hopefully speed up the review process.
> >
> > I have raised a JIRA for this [2] and, initially, this would probably just consist of calling the pre-compile checks workflow as is from a top level PR check workflow. We could then add any further checks we identify in the future to that workflow.
> >
> > Let me know what you think?
> >
> > Cheers,
> >
> > Tom Cooper
> > @tomcooper.dev | https://tomcooper.dev
> >
> > [1] https://github.com/apache/flink/blob/master/.github/workflows/template.pre-compile-checks.yml
> > [2] https://issues.apache.org/jira/browse/FLINK-37534
Re: CHI: Add liiniting checks to all PRs
Posted by Arvid Heise <ah...@confluent.io.INVALID>.
+1, this will save time for everyone.
On Fri, Mar 21, 2025 at 5:58 PM Tom Cooper <co...@tomcooper.dev> wrote:
>
> Hey All,
>
> I wanted to start a discussion on enabling linting checks, via a GitHub Action, on all PRs in the main Flink repository.
>
> Often when a user submits a PR they will wait for the CI to run in Azure DevOps and this will fail as they have forgotten to run spotless or checkstyle. However, in order to find this out, the author has to dig through the Azure DevOps UI and often parse the logs.
>
> We currently have a GitHub Action for running "pre-compile" checks that include all the standard linting checks: .github/workflows/template.pre-compile-checks.yml [1]. However this is only run as part of the nightly (beta) runs.
>
> We have discussed, in the CHI meetings, how it would be a better experience for developers if this was run for every PR (using the default JVM, currently 17). That way a clear indication would be given, via the GitHub UI (green ticks and red crosses), if the PR has passed the basic checks or not.
>
> This would hopefully speed up the review process.
>
> I have raised a JIRA for this [2] and, initially, this would probably just consist of calling the pre-compile checks workflow as is from a top level PR check workflow. We could then add any further checks we identify in the future to that workflow.
>
> Let me know what you think?
>
> Cheers,
>
> Tom Cooper
> @tomcooper.dev | https://tomcooper.dev
>
> [1] https://github.com/apache/flink/blob/master/.github/workflows/template.pre-compile-checks.yml
> [2] https://issues.apache.org/jira/browse/FLINK-37534