WIP - Refactoring analyzer tests to use custom docker registry #685
Conversation
…s with only one Signed-off-by: Juan Bustamante <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
…r registry helper Signed-off-by: Juan Bustamante <[email protected]>
acceptance/analyzer_test.go
Outdated
| @@ -1214,13 +1218,13 @@ func minifyMetadata(t *testing.T, path string, metadataStruct interface{}) strin | |||
| return string(flatMetadata) | |||
| } | |||
|
|
|||
| func buildAuthRegistryImage(t *testing.T, repoName, context string, buildArgs ...string) (string, string) { | |||
| func buildAuthRegistryImage(t *testing.T, repoName, context string, registry *ih.DockerRegistry, buildArgs ...string) (string, string) { | |||
There was a problem hiding this comment.
| func buildAuthRegistryImage(t *testing.T, repoName, context string, registry *ih.DockerRegistry, buildArgs ...string) (string, string) { | |
| func buildRegistryImage(t *testing.T, repoName, context string, registry *ih.DockerRegistry, buildArgs ...string) (string, string) { |
I wonder if it still makes sense to have this function return the auth. Since we're only getting the auth one time... maybe we could just pull that out, and then we don't have to ignore the second return value every other time.
There was a problem hiding this comment.
I did the change, you can take a look and give me your thoughts
|
@jjbustamante this is looking good! There are a few places where we are explicitly testing the auth, so I don't think we want to pass images that are going to bypass basic auth for the registry. We'll probably need to create a couple extra fixtures that don't have permissions set. |
|
Also it looks like there is a merge conflict now :( I hope it won't be too tedious to resolve. I am happy to pair on it if you'd like (I think the PR that introduced the conflict was mine). |
…po after the depending PR was merged Signed-off-by: Juan Bustamante <[email protected]>
| @@ -1230,7 +1254,7 @@ func buildAuthRegistryImage(t *testing.T, repoName, context string, registry *ih | |||
| authConfig, err := auth.BuildEnvVar(authn.DefaultKeychain, regRepoName) | |||
| h.AssertNil(t, err) | |||
|
|
|||
| return regRepoName, authConfig | |||
| return authConfig | |||
There was a problem hiding this comment.
Ahh I'm sorry, I think I was not clear in my earlier comment. I meant remove authConfig, and keep regRepoName!
There was a problem hiding this comment.
Mmm... but I think we need the Credentials created for the registry. right? it is used only once because it is the same registry, but it still a required data to fill the fixture
There was a problem hiding this comment.
Ohh you're right. The thing is, auth.BuildEnvVar - it expects to be called with a repo, but the auth is per registry, not per repo. Probably we should refactor auth.BuildEnvVar to just expect a registry, but that's another story...
In any case, I think you could call lines 1370+1371 outside of this function with the first repo that is created.
…er, but some tests are broken Signed-off-by: Juan Bustamante <[email protected]>
|
@natalieparellano I tried to fix the merging conflict, but now there are some tests failing, I think probably I missed something maybe we can try to check |
Signed-off-by: Juan Bustamante <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
References
Context
After merging PR #646 we agreed to refactor the analyzer tests and simplifying the number of docker registries we are using.
Description
authRegistryand thereadOnlyRegistryand replaces them with just 1testRegistrythat uses theImagePrivilgesapproach implemented in imgUtil repo PR #126 to configure the registry to handle the required behaviors.Depends on