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

fix: handle multiple imagePullSecrets #666

Merged
merged 3 commits into from
Jun 6, 2024
Merged

Conversation

toddbaert
Copy link
Member

@toddbaert toddbaert commented Jun 5, 2024

The previous iteration of this was ALMOST working, but:

  • it didn't support multiple secrets
  • it broke if you did a helm upgrade without a imagePullSecret, forcing the user to use --force.

I've added a sort of hacky "post-processor" script which allows us to add helm control flow where we otherwise couldn't. After talking with @thisthat I think this is the only solution until kustomize supports helm output.

Again, I tested locally, and ImagePullSecrets (even more than one) are added to both the manager deployment and the deployments we create at runtime.

Fixes: #665

@toddbaert toddbaert requested a review from a team as a code owner June 5, 2024 19:08
@toddbaert toddbaert force-pushed the fix/more-image-pull-secrets branch 2 times, most recently from 724fad4 to d88d310 Compare June 5, 2024 19:14
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 86.51%. Comparing base (546635e) to head (724fad4).
Report is 13 commits behind head on main.

Current head 724fad4 differs from pull request most recent head b84c1a7

Please upload reports for the commit b84c1a7 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #666      +/-   ##
==========================================
- Coverage   86.81%   86.51%   -0.30%     
==========================================
  Files          19       19              
  Lines        1577     1587      +10     
==========================================
+ Hits         1369     1373       +4     
- Misses        168      173       +5     
- Partials       40       41       +1     
Files Coverage Δ
common/flagdproxy/flagdproxy.go 88.70% <100.00%> (+0.53%) ⬆️
controllers/core/flagd/config.go 0.00% <0.00%> (ø)
controllers/core/flagd/resources/deployment.go 94.56% <0.00%> (-5.44%) ⬇️

... and 2 files with indirect coverage changes

Flag Coverage Δ
unit-tests 86.51% <50.00%> (-0.30%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@toddbaert toddbaert force-pushed the fix/more-image-pull-secrets branch from d88d310 to a2e0aa6 Compare June 5, 2024 19:18
Signed-off-by: Todd Baert <[email protected]>
@toddbaert toddbaert requested review from bacherfl and odubajDT June 5, 2024 21:22
Copy link
Contributor

@Kavindu-Dodan Kavindu-Dodan left a comment

Choose a reason for hiding this comment

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

I think this is a good solution to overcome the limitation 👍

Copy link
Member

@thisthat thisthat left a comment

Choose a reason for hiding this comment

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

Unfortunately, I believe this is the best solution we could use at the moment until there is a better support for Helm control flow strings in Kustomize :(

@beeme1mr beeme1mr merged commit df3d6d9 into main Jun 6, 2024
19 checks passed
@github-actions github-actions bot mentioned this pull request Jun 6, 2024
@toddbaert toddbaert deleted the fix/more-image-pull-secrets branch June 6, 2024 13:13
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.

Kustomize/Helm Cleanup / Support Multiple ImagePullSecrets
4 participants