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

Update imagepullsecrets in service account on existing clusters/serviceaccounts #384

Merged
merged 4 commits into from
Aug 26, 2021
Merged

Update imagepullsecrets in service account on existing clusters/serviceaccounts #384

merged 4 commits into from
Aug 26, 2021

Conversation

nikore
Copy link
Contributor

@nikore nikore commented Aug 25, 2021

Change log description

Updates the imagePullSecrets section of the ServiceAccount on reconciliation

What the code does

Noticed when working on #383 that if I updated the imagePullSecrets setting to use a private repo it wouldn't update the ServiceAccount. This meant I would have to either manually edit the ServiceAccount or delete the cluster and make a new one.

This allows updating the imagePullSecrets section in the ServiceAccount on existing clusters.

Purpose of the change

Fixes #388

How to verify it

Deploy a zookeeper cluster, then update spec.Pod.ImagePullSecrets with a new secret and wait for the cluster to reconcile. I also added a unit test covering this use case.

@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #384 (dbb8d58) into master (aafd465) will decrease coverage by 0.13%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #384      +/-   ##
==========================================
- Coverage   86.00%   85.86%   -0.14%     
==========================================
  Files          11       11              
  Lines        1572     1578       +6     
==========================================
+ Hits         1352     1355       +3     
  Misses        147      147              
- Partials       73       76       +3     
Impacted Files Coverage Δ
...er/zookeepercluster/zookeepercluster_controller.go 63.02% <50.00%> (-0.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aafd465...dbb8d58. Read the comment docs.

@anishakj
Copy link
Contributor

@nikore Please raise an issue corresponding to this. And looks like the newly added lines are not covered by unit tests. Could you please have a look?

@nikore
Copy link
Contributor Author

nikore commented Aug 26, 2021

The only thing that isn't covered is the return err part of that function which there is no way for me to reach given the current test setup. The current testing seems consistent with the other tests in this file.

Codecov does a bad job at catching conditional error handling like this, if I where to change it to check error in a different line it would report it better.

@derekm derekm merged commit 38a3da8 into pravega:master Aug 26, 2021
Copy link
Contributor

@anishakj anishakj left a comment

Choose a reason for hiding this comment

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

LGTM

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.

ImagePullSecrets section of the ServiceAccount isn't updated when there is an existing one
3 participants