-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add localhost registry to the set avoiding tag resolution as a workaround for #467 #468
Add localhost registry to the set avoiding tag resolution as a workaround for #467 #468
Conversation
pkg/install/install.go
Outdated
if err := runCommand(ignoreRegistry); err != nil { | ||
return fmt.Errorf("tag resolving configuration: %w", err) | ||
} | ||
fmt.Println(" Tag resolving configuration patched...") |
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.
I would add the why not so much the what or how. I.e. how it is patched, what does this change ?. Also not sure if the message itself might be too noisy and potentially confusing for a beginner who is running the quickstart. At the end it should just work, and a beginner should not worry about what a "tag resolving configuration" is.
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.
I'm happy to remove the comment if we want, it's hard to briefly explain the feature.
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.
Switched this to "enabling local registry deployments"
Thanks, looks good to me in general. For the Release notes I would add that this only affects |
agree, somebody probably should've done that 😟 😆 Part of me also thinks that perhaps we should switch the default here (i.e. create the registry by default but allow users to opt out by passing a |
I'd be happy to change the default for the |
From the earlier history, it looked like we disabled the registry flag due to Podman compatibility concerns, so we may want to check that before we re-enable it. |
👍 I'm on board with merging/cherry picking your fix as is. I had meant my comment as something possibly for the future, but turns out I was misremembering how the registry flag works (for some reason I was thinking it was required rather than an optional component), so let's just pretend I never said anything about that 😉 |
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: evankanderson, psschwei 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 |
/cherry-pick release-1.12 |
@psschwei: new pull request created: #469 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-1.11 |
@psschwei: new pull request created: #470 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Changes
registries-skipping-tag-resolving
setting for localhost registry when configured. This saves a lot of debugging for new folks, since the actual registry will be reachable atkind-registry:5000
in the containerd config, but serving doesn't have equivalent configuration./kind bug
Fixes #467
Release Note
Docs