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

CI improvements #396

Merged
merged 2 commits into from
Dec 28, 2023
Merged

CI improvements #396

merged 2 commits into from
Dec 28, 2023

Conversation

shimritproj
Copy link
Contributor

@shimritproj shimritproj commented Oct 18, 2023

The CI runs different checks that validates stuff such as: commit message, unit-tests, images built, e2e tests, backward compatibility tests, and so on.
The checks are split into workflows.
For example, see workflows runs here: https://github.com/metallb/metallb/actions

The workflows running are defined under the /.github/workflows dir.

The goal of this story is to improve metallb-operator's CI by bringing few features we have in metallb's CI.

The features we want to bring are:

  1. combine workflows under one workflow, leveraging the "needs" option for code reuse and requiring successful dependent jobs, thus improving readability.
  2. merge_group: this is an event we add in the workflow yaml and it allows us to have merge queue that speeds up the process of merging PRs. see more here
  3. composite: we use a composite action to reduce duplication, currently
    we have composite for setup and for collecting logs. we can benefit from it
    also in the metallb-operator CI.

@shimritproj shimritproj force-pushed the Updated_ci.yaml_file branch 2 times, most recently from 8b32388 to 7471453 Compare October 18, 2023 11:40
Copy link
Contributor

@liornoy liornoy left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I added a couple of comments, some are just proposing renaming,
notice that some comments apply also to other places in the yaml that I didn't repeat.
Also, please add commit message that describes what and why we're doing in this change, you can take inspiration from the description in Jira.

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/composite/collectlogs/action.yaml Outdated Show resolved Hide resolved
.github/workflows/composite/setup/action.yaml Outdated Show resolved Hide resolved
.github/workflows/composite/setup/action.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@shimritproj shimritproj requested a review from liornoy October 19, 2023 14:00
@shimritproj
Copy link
Contributor Author

shimritproj commented Oct 19, 2023

Thank you for the PR! I added a couple of comments, some are just proposing renaming, notice that some comments apply also to other places in the yaml that I didn't repeat. Also, please add commit message that describes what and why we're doing in this change, you can take inspiration from the description in Jira.

Thanks for the excellent review, I solved the comments. @liornoy

@shimritproj shimritproj force-pushed the Updated_ci.yaml_file branch 3 times, most recently from 3ed7b6d to a7c21b6 Compare October 19, 2023 15:32
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@shimritproj shimritproj force-pushed the Updated_ci.yaml_file branch 3 times, most recently from 9e9c108 to c6bdfaa Compare October 22, 2023 14:31
@shimritproj shimritproj changed the title Updated ci.yaml file CI improvements Oct 22, 2023
@shimritproj shimritproj force-pushed the Updated_ci.yaml_file branch 12 times, most recently from 710e65c to 722646b Compare October 25, 2023 13:59
@shimritproj shimritproj force-pushed the Updated_ci.yaml_file branch 6 times, most recently from 9e59136 to 0a22ca9 Compare November 28, 2023 19:56
@liornoy
Copy link
Contributor

liornoy commented Dec 4, 2023

@oribon I see we hit the known verify bundle flake in the olm lane. can you please re-run it?

@shimritproj shimritproj force-pushed the Updated_ci.yaml_file branch 10 times, most recently from 2453c2c to 73d71a4 Compare December 26, 2023 13:52
@shimritproj shimritproj force-pushed the Updated_ci.yaml_file branch 2 times, most recently from aec8d1d to 8c0f285 Compare December 26, 2023 16:18
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Show resolved Hide resolved
Comment on lines 80 to 79
- name: Verify manifests
run: |
make manifests
git diff --exit-code
Copy link
Member

Choose a reason for hiding this comment

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

+1

.github/workflows/ci.yaml Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/composite/setup/action.yaml Outdated Show resolved Hide resolved
@shimritproj shimritproj force-pushed the Updated_ci.yaml_file branch 3 times, most recently from 9912ada to f691382 Compare December 27, 2023 13:46
1. Combine workflows (general, go, metallb_e2e, olm, upgrade)
 under one workflow, leveraging the "needs"  option for code reuse
and requiring successful dependent jobs, thus improving readability.
2. Add merge_group.
3. Add composite.

Signed-off-by: shimritproj <[email protected]>
Adding artifact names for each collected log will help us understand which logs are suitable for each job.
In the end, we will be able to see links in the GitHub UI for all artifact names, and there, all the logs suitable for each job will be accessible.

Signed-off-by: shimritproj <[email protected]>
Copy link
Member

@oribon oribon left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@oribon oribon merged commit bfd4696 into metallb:main Dec 28, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants