registry: use generated imagestream clientset to retrieve secrets #16098
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mfojtik The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
openshift-merge-robot
added
approved
openshift-ci-robot
added
the
size/XXL
| // NewFakeOpenShiftWithClient constructs a fake client associated with | ||
| // the stateful fake in-memory OpenShift reactors. The fake OpenShift is | ||
| // available for direct interaction, so you can make buggy states. | ||
| // TODO: remove the FakeOpenshift as the legacy client is not needed anymore |
There was a problem hiding this comment.
@mfojtik I guess you are about testclient.Fake, not FakeOpenShift.
There was a problem hiding this comment.
testclient.Fake is not longer needed in registry, we should use imagefake.Clientset{} from generated client (similar to other clients required for openshift resources....).
|
(pls do not tag until the dependency PR merges) |
mfojtik
force-pushed
the
registry-imagestreamsecrets
branch
from
c4644e1 to
43b26b0
Compare
| } | ||
| } | ||
|
|
||
| secretsv1 := make([]kapiv1.Secret, len(s.secrets)) |
There was a problem hiding this comment.
round two... make the caller deal with conversion, honor callers that use external version (like registry).
mfojtik
force-pushed
the
registry-imagestreamsecrets
branch
from
43b26b0 to
209a08d
Compare
mfojtik
force-pushed
the
registry-imagestreamsecrets
branch
from
209a08d to
906cbfd
Compare
openshift-ci-robot
added
size/L
|
|
||
| func (c *fakeRegistryClient) Client() (Interface, error) { | ||
| return newAPIClient(c.client, nil, nil, c.images, nil), nil | ||
| return newAPIClient(nil, nil, c.images, nil), nil |
There was a problem hiding this comment.
@dmage as part of the test client refactoring we should get rid of all this nils and just pass the clients we actually use (I believe we use just images client).
There was a problem hiding this comment.
We just don't have unit tests for the paths with other clients (e.g., limit ranges). If we have better code coverage, we will not have nils there.
| return nil, err | ||
| } | ||
| return secrets.Items, nil | ||
| secretsv1 := make([]kapiv1.Secret, len(secrets.Items)) |
There was a problem hiding this comment.
@soltysh this might be interesting for you... this is needed to allow registry use the credentials (registry use external client). when we switch to external client in image controller, this can be nuked.
|
/retest |
2 similar comments
|
/retest |
|
/retest |
| // ClientFromToken returns the client based on the bearer token. | ||
| func (c *registryClient) ClientFromToken(token string) (Interface, error) { | ||
| newClient := *c | ||
| newOpenshiftConfig := clientcmd.AnonymousClientConfig(newClient.openshiftConfig) |
There was a problem hiding this comment.
Please use k8s.io/client-go/rest.AnonymousClientConfig
| for i, secret := range secrets.Items { | ||
| err := kapiv1.Convert_api_Secret_To_v1_Secret(&secret, &secretsv1[i], nil) | ||
| if err != nil { | ||
| glog.V(2).Infof("Unable to make the Docker keyring for %s/%s secret: %v", secret.Name, secret.Namespace, err) |
|
Two comments. After they are fixed, lgtm. |
mfojtik
force-pushed
the
registry-imagestreamsecrets
branch
from
906cbfd to
a96b157
Compare
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
|
Automatic merge from submit-queue (batch tested with PRs 16098, 16155) |
10 participants
@deads2k this is making registry using purely external client.