Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add status check jobs for local_e2e_tests matrix and linters #5628

Merged
merged 9 commits into from
Nov 14, 2022

Conversation

beni0888
Copy link
Collaborator

Description of the change

Due to GH limitations, it is not possible to add a branch protection rule pointing to all the jobs generated by GHA's matrix strategy. As a workaround, we've introduced a new status check job that depends on the job with the matrix, and whose status depends on the success of all the jobs generated by that. In addition, we've taken advantage to add another check-status job for all the linter jobs.

Benefits

  • Simplify the branch protection rules configuration, being able to select just one status check that covers all the local_e2e_tests jobs derived from the matrix.
  • The same for the linter jobs, rely on just one status check covering them all.

Possible drawbacks

None

Applicable issues

Additional information

Jesús Benito Calzada added 2 commits November 10, 2022 13:07
Signed-off-by: Jesús Benito Calzada <bjesus@vmware.com>
Signed-off-by: Jesús Benito Calzada <bjesus@vmware.com>
@netlify
Copy link

netlify bot commented Nov 10, 2022

Deploy Preview for kubeapps-dev canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 2dfd43f
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/636e6a5a9fc2af0008ec6162

@beni0888 beni0888 requested a review from absoludity November 10, 2022 12:33
@beni0888 beni0888 self-assigned this Nov 10, 2022
@beni0888 beni0888 added component/ci Issue related to kubeapps ci system github_actions Pull requests that update GitHub Actions code kind/enhancement An issue that reports an enhancement for an implemented feature labels Nov 10, 2022
@beni0888 beni0888 added this to the Migrate CI to GitHub Actions milestone Nov 10, 2022
@beni0888 beni0888 marked this pull request as ready for review November 10, 2022 12:34
Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation!

Jesús Benito Calzada added 4 commits November 10, 2022 15:54
Signed-off-by: Jesús Benito Calzada <bjesus@vmware.com>
Signed-off-by: Jesús Benito Calzada <bjesus@vmware.com>
Signed-off-by: Jesús Benito Calzada <bjesus@vmware.com>
Signed-off-by: Jesús Benito Calzada <bjesus@vmware.com>
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

Comment on lines 58 to 63
linters-status-check:
needs:
- yaml-linter
- golang-linter
- license-headers-linter
- codeql-linter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, don't we already run the yaml and golang linters during the image builds (as a step in the docker file build, so that people can and will find the lint problem locally). The headers linter and codeql linters probably need to be run separately here, just not sure about the yaml and golang ones?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IINM the golang linter is not being run as part of the image build, it is true that golangci-lint is being installed in the docker file, but its execution is conditioned to the existence of a build argument called lint that it's currently not being passed in the call to docker build that is made in the Makefile. Regarding yamllint, I've added it recently, so it's never being executed as part of the image build process. IMHO, it makes sense to have a listing step in the pipeline itself that allows us to easily identify that the issue is in the linting step. Besides that, and taking into account that not all the linters could be run as part of the image-building process, I think it's better to have all the linters aggregated as part of the same process instead of having them scattered through different places. Don't you think so?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IINM the golang linter is not being run as part of the image build, it is true that golangci-lint is being installed in the docker file, but its execution is conditioned to the existence of a build argument called lint that it's currently not being passed in the call to docker build that is made in the Makefile.

Oh - yes, it looks like in #5234, 2 months ago, @antgamdia updated the Dockerfile to make it conditional . Do you remember why it was made conditional @antgamdia?

Regarding yamllint, I've added it recently, so it's never being executed as part of the image build process.

👍 ah - ok.

IMHO, it makes sense to have a listing step in the pipeline itself that allows us to easily identify that the issue is in the linting step. Besides that, and taking into account that not all the linters could be run as part of the image-building process, I think it's better to have all the linters aggregated as part of the same process instead of having them scattered through different places. Don't you think so?

I agree in part, in that I have no problem with a lint stage in CI per-se. The key thing for me is that we avoid us devs doing:

  1. Commit and create PR,
  2. Find next task and start reading about it
  3. Notice failed lint in CI for (1) so switch back
    (repeat).

So the key thing is, IMO, that we ensure the feedback is fast (ie. at worst, run early in CI which I'm sure you're doing), and that we can run the same check easily locally. The purpose of including such things in the build step of the Dockerfile is because it's an easy way for fast feedback and simple local running.

+1 either way :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I see your point. The linters stage is executed right after starting the pipeline, so the feedback look will be short. Regarding running the linters locally, we could add a make target for that, although that would be out of the scope of this PR I guess.

Jesús Benito Calzada added 3 commits November 11, 2022 07:22
Signed-off-by: Jesús Benito Calzada <bjesus@vmware.com>
Signed-off-by: Jesús Miguel Benito Calzada <bjesus@vmware.com>
@beni0888 beni0888 merged commit d494f03 into vmware-tanzu:main Nov 14, 2022
@beni0888 beni0888 deleted the aggregate-local-e2e-tests-result branch November 14, 2022 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-not-required component/ci Issue related to kubeapps ci system github_actions Pull requests that update GitHub Actions code kind/enhancement An issue that reports an enhancement for an implemented feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants