Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

Image pull secrets #411

Merged
merged 2 commits into from Apr 7, 2020
Merged

Image pull secrets #411

merged 2 commits into from Apr 7, 2020

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Apr 6, 2020

Allow setting image pull secrets for service accounts to enable pulling the consul-k8s and consul images from private registries.

Kube docs: https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#add-imagepullsecrets-to-a-service-account

Notes

@lkysow lkysow changed the base branch from wan-federation to master April 6, 2020 19:11
@lkysow lkysow marked this pull request as ready for review April 6, 2020 19:11
@lkysow lkysow requested a review from a team April 6, 2020 19:12
@lkysow lkysow added area/chart-only Related to changes that simply require yaml chart changes, e.g. exposing a new field enhancement New feature or request labels Apr 6, 2020
@lkysow lkysow force-pushed the image-pull-secrets branch 2 times, most recently from 326b6a7 to d572ecb Compare April 6, 2020 19:54
Copy link
Contributor

@adilyse adilyse left a comment

Choose a reason for hiding this comment

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

Looking good. The one thing I noticed is that the value is defined as an array, but all of the tests only test a single value. It'd be good to make sure that the iteration through multiple values works as expected too.

values.yaml Outdated
#
# Example:
# imagePullSecrets:
# - name: my-secret-name
Copy link
Contributor

Choose a reason for hiding this comment

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

These example feels confusing. When empty, the value needs [], but it doesn't need them when I give a value? In the extraConfig text we mention that it's an array of objects, which might help a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah good call, changed to say "array of objects containing image pull secret names"

@lkysow
Copy link
Member Author

lkysow commented Apr 7, 2020

The one thing I noticed is that the value is defined as an array, but all of the tests only test a single value. It'd be good to make sure that the iteration through multiple values works as expected too.

For sure. I've changed the tests to set two image pull secrets and assert that both are set.

@lkysow lkysow requested a review from adilyse April 7, 2020 18:47
Copy link
Contributor

@adilyse adilyse left a comment

Choose a reason for hiding this comment

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

Generally when we're testing for several values, each one is checked separately. Example: env variable

@lkysow lkysow requested a review from adilyse April 7, 2020 19:28
Copy link
Contributor

@adilyse adilyse left a comment

Choose a reason for hiding this comment

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

One more question-- should these be quoted? From the docs, it looks like they're not being quoted:

imagePullSecrets:
- name: myregistrykey

@lkysow
Copy link
Member Author

lkysow commented Apr 7, 2020

It doesn't matter, it's just a string.

Copy link
Contributor

@adilyse adilyse left a comment

Choose a reason for hiding this comment

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

Looks good.

Though if the quoting doesn't matter, it's a little odd to go to the effort to add it.

@lkysow
Copy link
Member Author

lkysow commented Apr 7, 2020

Yeah I'll remove it.

Allow configuring image pull secrets via service account. To use, set
the helm config
  global:
    imagePullSecrets:
    - name: my-secret-with-image-pull-secrets
@lkysow lkysow merged commit 27781ef into master Apr 7, 2020
@lkysow lkysow deleted the image-pull-secrets branch April 7, 2020 20:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/chart-only Related to changes that simply require yaml chart changes, e.g. exposing a new field enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add imagePullSecrets to DaemonSets
3 participants