Skip to content

fix: added missing pod labels for init job - #30956

Merged
villebro merged 2 commits into
apache:masterfrom
glothriel:missing-pod-labels-for-init-job
Dec 9, 2024
Merged

fix: added missing pod labels for init job#30956
villebro merged 2 commits into
apache:masterfrom
glothriel:missing-pod-labels-for-init-job

Conversation

@glothriel

@glothriel glothriel commented Nov 18, 2024

Copy link
Copy Markdown
Contributor

SUMMARY

Added missing pod labels for init job. Without them it's impossible to dynamically set labels in environments with Network Policies engaged.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

skip

TESTING INSTRUCTIONS

Running helm template will suffice.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@dosubot dosubot Bot added the deploy:helm label Nov 18, 2024
github-actions[bot]
github-actions Bot reviewed Nov 18, 2024
View reviewed changes

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

villebro
villebro reviewed Nov 18, 2024
View reviewed changes

@villebro villebro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the fix, LGTM. However, I noticed we're also missing the standard metadata.labels on this template that are found in the other ones (app, chart, release, heritage) - could you add those while we're at it?

@glothriel
glothriel requested a review from villebro November 19, 2024 08:52
@glothriel

glothriel commented Nov 19, 2024

Copy link
Copy Markdown
Contributor Author

@villebro Some pre-check (helm docs modified?) failed for the previous commit, should I perform some additional action?

@villebro

villebro commented Nov 19, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

@glothriel you need to run helm-docs to rebuild README.md, as it's missing init.podLabels (check pre-commit hooks, which does this automatically). Also, please bump the version to 0.13.3, as 0.13.2 was just released.

@villebro

villebro commented Nov 22, 2024

Copy link
Copy Markdown
Member

@glothriel this PR requires yet another rebase, as we're now already on version 0.13.3. Also, note that the version was not updated in the README in the latest commits, so you may want to consider installing pre-commit to automatically sync the README when adding new commits.

@glothriel
glothriel force-pushed the missing-pod-labels-for-init-job branch from 9758a50 to 65402a3 Compare November 27, 2024 10:55
@glothriel

glothriel commented Nov 27, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Ok guys i hope i got it this time
image

@glothriel

glothriel commented Dec 2, 2024

Copy link
Copy Markdown
Contributor Author

@villebro Can I ask for review please?

@akkbng

akkbng commented Dec 6, 2024

Copy link
Copy Markdown

hey @villebro would it be possible to get this PR reviewed soon?

villebro
villebro approved these changes Dec 6, 2024
View reviewed changes

@villebro villebro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your patience! ❤️

villebro
villebro requested changes Dec 6, 2024
View reviewed changes

@villebro villebro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One more change request to make this feature complete - sorry for one more iteration 🙁

Comment thread helm/superset/templates/init-job.yaml Outdated
@glothriel
glothriel requested a review from villebro December 9, 2024 07:37
villebro
villebro approved these changes Dec 9, 2024
View reviewed changes

@villebro villebro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, hopefully the final one 😁

@villebro
villebro merged commit b54a97b into apache:master Dec 9, 2024
@villebro villebro mentioned this pull request Dec 9, 2024
Closed
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 5.0.0 First shipped in 5.0.0 labels Jul 29, 2025
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
@glothriel
fix: added missing pod labels for init job (apache#30956)
4589d54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@villebro villebro villebro approved these changes
@github-actions github-actions[bot] github-actions[bot] left review comments
@craig-rueda craig-rueda Awaiting requested review from craig-rueda
@dpgaspar dpgaspar Awaiting requested review from dpgaspar dpgaspar is a code owner
@nytai nytai Awaiting requested review from nytai nytai is a code owner
@michael-s-molina michael-s-molina Awaiting requested review from michael-s-molina michael-s-molina is a code owner
+1 more reviewer
@maxforasteiro maxforasteiro maxforasteiro approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels deploy:helm size/S 🚢 5.0.0 First shipped in 5.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@glothriel @villebro @akkbng @maxforasteiro @mistercrunch