Skip to content

Remove appArmorProfile from CronJob helm template #34100

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 9, 2024
Merged

Remove appArmorProfile from CronJob helm template #34100

merged 1 commit into from
Aug 9, 2024

Conversation

sathieu
Copy link
Contributor

@sathieu sathieu commented Jul 31, 2024 •
edited
Loading

The condition is reversed, and fixing it breaks CI.

See #33077

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

Fixes: #34145

Fix appArmorProfile condition for CronJob helm template

@sathieu sathieu requested review from a team as code owners July 31, 2024 06:05
@sathieu sathieu requested review from joamaki, nebril and rolinh July 31, 2024 06:05
@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 Jul 31, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jul 31, 2024
@sathieu sathieu mentioned this pull request Jul 31, 2024
Merged
8 tasks
rolinh
rolinh approved these changes Jul 31, 2024
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Nice catch!

@rolinh rolinh added release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch labels Jul 31, 2024
@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 Jul 31, 2024
@rolinh
Copy link
Member

rolinh commented Jul 31, 2024

/test

nebril
nebril approved these changes Aug 1, 2024
View reviewed changes
Copy link
Member

@nebril nebril left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR! If you say so.

@rolinh rolinh removed the request for review from joamaki August 1, 2024 12:30
@rolinh rolinh mentioned this pull request Aug 5, 2024
Closed
3 tasks
@rolinh rolinh enabled auto-merge August 5, 2024 08:56
@sathieu
Copy link
Contributor Author

sathieu commented Aug 5, 2024

@rolinh thanks for enabling automerge. However, the CI is failing (probably for unrelated reason). How to move forward?

@rolinh
Copy link
Member

rolinh commented Aug 6, 2024

/ci-clustermesh

@rolinh
Copy link
Member

rolinh commented Aug 6, 2024

@sathieu, not sure but agree, it's probably a flake. I re-triggered the failing test, let's see if that suffices.

auto-merge was automatically disabled August 6, 2024 11:46

Head branch was pushed to by a user without write access

@sathieu
Copy link
Contributor Author

sathieu commented Aug 6, 2024

@sathieu, not sure but agree, it's probably a flake. I re-triggered the failing test, let's see if that suffices.

CI is still failing. I rebased...

@sathieu sathieu changed the title Fix appArmorProfile condition for CronJob helm template Remove appArmorProfile from CronJob helm template Aug 6, 2024
@sathieu
Copy link
Contributor Author

sathieu commented Aug 6, 2024 •
edited
Loading

@rolinh I ended up removing completely the appArmorProfile. This was breaking CI (probably a kind known issue).

Please review again 🙏 .

EDIT: this is actually reverting badf925.

@sathieu
Remove appArmorProfile from CronJob helm template
df5352a 
The condition is reversed, and fixing it breaks CI.

See cilium#33077

Signed-off-by: Mathieu Parent <math.parent@gmail.com>
@julianwiedmann
Copy link
Member

/test

@julianwiedmann julianwiedmann added the dont-merge/waiting-for-review Requires further review before merging. label Aug 9, 2024
@julianwiedmann
Copy link
Member

julianwiedmann commented Aug 9, 2024 •
edited
Loading

Please review again

@rolinh blocking on this, in case you want to have another look

@sathieu
Copy link
Contributor Author

sathieu commented Aug 9, 2024

@julianwiedmann There are some CI failures, but I don't understand the problem. Maybe a flaky test?

@aanm aanm enabled auto-merge August 9, 2024 12:13
@aanm aanm disabled auto-merge August 9, 2024 12:13
@aanm aanm requested a review from rolinh August 9, 2024 12:13
rolinh
rolinh approved these changes Aug 9, 2024
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. LGTM, thanks!

@rolinh rolinh removed the dont-merge/waiting-for-review Requires further review before merging. label Aug 9, 2024
@rolinh rolinh added this pull request to the merge queue Aug 9, 2024
Merged via the queue into cilium:main with commit 55c44d6 Aug 9, 2024
68 checks passed
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Aug 9, 2024
@jschwinger233 jschwinger233 mentioned this pull request Aug 12, 2024
Merged
15 tasks
@jschwinger233 jschwinger233 added backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. and removed needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch labels Aug 12, 2024
@github-actions github-actions bot added backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. and removed backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. labels Aug 13, 2024
@cilium-release-bot cilium-release-bot bot mentioned this pull request Aug 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rolinh rolinh rolinh approved these changes

@nebril nebril nebril approved these changes

Assignees
No one assigned
Labels
backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. 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/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Helm chart fails to apply on Kubernetes <1.30 if hubble.tls.auto.method=cronJob
5 participants
@sathieu @rolinh @julianwiedmann @nebril @jschwinger233