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

workflows: various small fixes #16311

Merged
merged 2 commits into from Jun 2, 2021

Conversation

nbusseneau
Copy link
Member

Please see individual commits.

@nbusseneau nbusseneau added area/CI Continuous Integration testing issue or flake release-note/ci This PR makes changes to the CI. labels May 25, 2021
@nbusseneau nbusseneau requested review from a team as code owners May 25, 2021 22:03
@nbusseneau nbusseneau requested review from tklauser and aanm May 25, 2021 22:03
@nbusseneau nbusseneau requested a review from pchaigno May 25, 2021 22:04
.github/workflows/images-legacy.yaml Outdated Show resolved Hide resolved
@nbusseneau nbusseneau force-pushed the pr/workflows-various-fixes branch 2 times, most recently from 8d4358a to 309ff77 Compare May 25, 2021 22:33
@nbusseneau
Copy link
Member Author

nbusseneau commented May 25, 2021

Before

After

Can't give you an after for images-legacy.yaml as it runs from master due to pull_request_target, though...

@@ -171,7 +171,7 @@ jobs:
uses: actions/upload-artifact@ee69f02b3dfdecd58bb31b4d133da38ba6fe3700
with:
name: image-digest ${{ matrix.name }}
path: image-digest
path: image-digest/${{ matrix.name }}.txt
Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify in the commit message what is wrong with the current path?

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course. You actually did well to ask me that because looking over it again: I think I made a mistake in my initial analysis.

I thought the upload artifacts was using the wrong path, which caused the warning here, but now I actually think the root cause is not the artifacts path but the fact we don't build artifacts for build-and-push-with-qemu for PR events, only for push events.

I pushed a more proper fix -- PTAL.

Copy link
Member Author

Choose a reason for hiding this comment

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

For context following offline discussion:

  • build-and-push-with-qemu builds cilium-test, which is used by the nightly.yaml workflow.
  • Nightly runs only a schedule, hence why the current setup in images-legacy.yaml only builds for pushes to master.
  • In the past, there was a nightly Jenkins job that could be triggered by test-nightly on PRs. It has since been marked as obsolete, probably when we moved nightly tests from Jenkins to GitHub, and removing the ability to test PRs in the process: https://jenkins.cilium.io/view/Obsolete/job/Cilium-PR-Nightly/

@nbusseneau nbusseneau force-pushed the pr/workflows-various-fixes branch 3 times, most recently from fd40aff to 421b5cc Compare May 26, 2021 18:00
@nbusseneau nbusseneau requested a review from aanm May 26, 2021 18:00
@nbusseneau nbusseneau requested a review from pchaigno May 26, 2021 18:01
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

changes LGTM but will be waiting on @nebril to look at the cilium-test image related changed

The build-and-push-with-qemu job was split and copied from the above
build-and-push-prs job, however it does not include building steps for
PR events, but only for pushes to master.

We rewrite the workflow to directly skip the build-and-push-with-qemu on
PR branches and streamline it since we only consider push events.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
As documented here:
https://github.com/dorny/paths-filter/blob/78ab00f87740f82aec8ed8826eb4c3c851044126/action.yml

> This option is ignored if action is triggered by pull_request event.

As for `${{ github.event.before }}`, it is undeeded for push events to `master`:

> Defaults to repository default branch (e.g. master).

They actually create a warning in GitHub actions, so I propose removing
it.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
@nebril nebril added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 1, 2021
@nathanjsweet nathanjsweet merged commit 652da82 into cilium:master Jun 2, 2021
@nbusseneau nbusseneau deleted the pr/workflows-various-fixes branch July 5, 2021 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration testing issue or flake ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants