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

Correctly use cli installer action in ipv4/6 smoke #28661

Merged
merged 1 commit into from Oct 18, 2023

Conversation

bleggett
Copy link
Contributor

Fix for regression in smoke test script introduced by #25493

See https://github.com/cilium/cilium/pull/25493/files#r1362783422

@bleggett bleggett requested review from a team as code owners October 17, 2023 21:32
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 17, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Oct 17, 2023
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Thanks! Looks like .github/workflows/tests-smoke.yaml could do with the same treatment.

@joestringer joestringer added the release-note/ci This PR makes changes to the CI. label Oct 17, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 17, 2023
@bleggett bleggett changed the title Correctly use cli installer action in ipv6 smoke Correctly use cli installer action in ipv4/6 smoke Oct 17, 2023
Copy link
Member

@joestringer joestringer 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, could you rebase to main / drop the merge commit?

@joestringer
Copy link
Member

joestringer commented Oct 17, 2023

^ Context, we try to keep Cilium git log linear for easier bisectability, and keep the commit messages simple and well-written so that future contributors can read back into the history and understand the context of the changes. The original commit message is good from that perspective, but the second one lacks context. Perhaps squash them all together during the rebase and just reword the commit to drop the IPv6 part?

@bleggett bleggett force-pushed the bleggett/use-action-ipv6-smoke branch from 0c63f02 to f702f99 Compare October 17, 2023 23:13
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

LGTM

@nbusseneau
Copy link
Member

Both affected workflows passed, no need to run the rest of the CI on this one, marking ready to merge.

@nbusseneau nbusseneau added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 18, 2023
@joestringer joestringer merged commit a136b57 into cilium:main Oct 18, 2023
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/community-contribution This was a contribution made by a community member. 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

3 participants