add internal and external URL handling for the docker pull secret #19838
Conversation
openshift-ci-robot
added
approved
|
This is super small. /approve |
|
But needs tests to make sure it doesn't break. |
| } | ||
| if len(in.ImagePolicyConfig.InternalRegistryHostname) > 0 { | ||
| registryURLs = append(registryURLs, in.ImagePolicyConfig.InternalRegistryHostname) | ||
| } |
There was a problem hiding this comment.
should this also check OPENSHIFT_DEFAULT_REGISTRY since that is also a valid (if deprecated?) way to set the url on the master?
There was a problem hiding this comment.
(that's an env var, sorry i wasn't clearer)
There was a problem hiding this comment.
no, we should never use it again. That's supposed to be set up front. Ansible forcibly upgrades you away.
There was a problem hiding this comment.
i think OPENSHIFT_DEFAULT_REGISTRY is handled by the logic that sets the ImagePolicyConfig already if I remember correctly
More than the unit test that makes sure the controller works? |
| @@ -1574,6 +1575,11 @@ type ServiceAccountControllerConfig struct { | |||
| ManagedNames []string | |||
| } | |||
|
|
|||
| type DockerPullSecretControllerConfig struct { | |||
| // RegistryURLs is a list of urls that the docker pull secrets should be valid for. | |||
| RegistryURLs []string | |||
There was a problem hiding this comment.
nit: optional: can we call this AdditionalRegistryURLs to match the controller field?
There was a problem hiding this comment.
nit: optional: can we call this AdditionalRegistryURLs to match the controller field?
I think from the config side, the user see this as his spot to specify the registry urls, not to set extra ones. From his point of view, we're the ones adding extras. The API is for them, not us.
| @@ -98,6 +102,9 @@ func NewDockerRegistryServiceController(secrets informers.SecretInformer, servic | |||
| type DockerRegistryServiceController struct { | |||
| client kclientset.Interface | |||
|
|
|||
| // AdditionalRegistryURLs is a list of URLs that are always included | |||
| @@ -218,7 +225,7 @@ func (e *DockerRegistryServiceController) watchForDockerURLChanges() { | |||
|
|
|||
| // getDockerRegistryLocations returns the dns form and the ip form of the secret | |||
| func (e *DockerRegistryServiceController) getDockerRegistryLocations() []string { | |||
| ret := []string{} | |||
| ret := append([]string{}, e.additionalRegistryURLs...) | |||
There was a problem hiding this comment.
why not start with ret := e.additionalRegistryURLs ?
There was a problem hiding this comment.
why not start with ret := e.additionalRegistryURLs ?
fear of accidental mutation.
deads2k
force-pushed
the
controller-25-pullsecret
branch
from
6abb9e4 to
dad692c
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, smarterclayton The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
size/L
deads2k
force-pushed
the
controller-25-pullsecret
branch
from
dad692c to
8558ecf
Compare
|
comments addressed, integration test added. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@deads2k the integration failure seems real. |
openshift-bot
added
the
needs-rebase
deads2k
force-pushed
the
controller-25-pullsecret
branch
from
8558ecf to
f3ceaee
Compare
openshift-bot
removed
the
needs-rebase
|
New changes are detected. LGTM label has been removed. |
deads2k
added
the
needs-rebase
|
/retest |
deads2k
added
lgtm
|
/retest |
1 similar comment
|
/retest |
This plumbs the information from the master config down to the controllers for internal and external registry values.
@smarterclayton it's small in case you change your mind.
@mfojtik we'll want test coverage on this (beyond my unit test). We need a card?
@bparees I saw your name somewhere