-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 the registry server we test against from 2.6 to 2.8 #15122
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mtrmac 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 |
Change LGTM, but two tests are failing that might be related to this change.
|
htpasswd is no longer included in docker.io/library/distribution after 2.7.0, per distribution/distribution-library-image#107 , and we want to upgrade to a recent version. At least system tests currently execute htpasswd from the OS, so it seems that it is likely to be available. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Those are related — |
Full disclosure: I hit “re-run test” a few times on things that looked like timing issues or not related. I’m not sure if that is common in this project or frowned upon. Notably at least one of the runs reported a flake on “podman push to local registry with authorization” — the registry logged a successful startup but subsequent accesses were refused, but the in-test re-run succeeded. I don’t think that’s a newly introduced bug but it’s possible that the change now makes some pre-existing race more likely (because we now just run a single external command instead of starting a container) |
LGTM |
@vrothberg @giuseppe @flouthoc PTAL |
test/e2e/login_logout_test.go
Outdated
"-e", strings.Join([]string{"REGISTRY_HTTP_ADDR=0.0.0.0", strconv.Itoa(port)}, ":"), "--name", "registry", "-v", | ||
strings.Join([]string{authPath, "/auth:Z"}, ":"), "-e", "REGISTRY_AUTH=htpasswd", "-e", | ||
"REGISTRY_AUTH_HTPASSWD_REALM=Registry Realm", "-e", "REGISTRY_AUTH_HTPASSWD_PATH=/auth/htpasswd", | ||
"-v", strings.Join([]string{certPath, "/certs:Z"}, ":"), "-e", "REGISTRY_HTTP_TLS_CERTIFICATE=/certs/domain.crt", | ||
"-e", "REGISTRY_HTTP_TLS_KEY=/certs/domain.key", "registry:2.6"}) | ||
"-e", "REGISTRY_HTTP_TLS_KEY=/certs/domain.key", "registry:2.8"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use the REGISTRY_IMAGE variable
test/e2e/login_logout_test.go
Outdated
@@ -249,7 +249,7 @@ var _ = Describe("Podman login and logout", func() { | |||
strings.Join([]string{authPath, "/auth:z"}, ":"), "-e", "REGISTRY_AUTH=htpasswd", "-e", | |||
"REGISTRY_AUTH_HTPASSWD_REALM=Registry Realm", "-e", "REGISTRY_AUTH_HTPASSWD_PATH=/auth/htpasswd", | |||
"-v", strings.Join([]string{certPath, "/certs:z"}, ":"), "-e", "REGISTRY_HTTP_TLS_CERTIFICATE=/certs/domain.crt", | |||
"-e", "REGISTRY_HTTP_TLS_KEY=/certs/domain.key", "registry:2.6"}) | |||
"-e", "REGISTRY_HTTP_TLS_KEY=/certs/domain.key", "registry:2.8"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed both (and also hack/podman-registry
).
... instead of hard-coding a copy of the value. Notably this makes hack/podman_registry actually support the documented -i option. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... primarily so that it can support OCI artifacts. 2.8 already seems to exist in the repo. This requires changing WaitContainerReady to also check stderr (ultimately because docker/distribution was updated to a more recent sirupsen/logrus, which logs by default to stderr instead of stdout). Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Note that this PR is already part of #15108, so I am closing this one here to delay our planet's dooms day by a nano second or two. |
... primarily so that it can support OCI artifacts.
2.8 already seems to exist in the repo.
Does this PR introduce a user-facing change?