Skip to content
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

warn and block old repo URLs #8903

Merged
merged 1 commit into from
Oct 19, 2020

Conversation

technosophos
Copy link
Member

Signed-off-by: Matt Butcher matt.butcher@microsoft.com

What this PR does / why we need it:

This is the least heavy-handed way I could come up with to stop people from using the old charts repo.

  • It warns if stable or incubator are mapped to one of the old URLs (but allows someone to, say, map them to oldstable)
  • It refuses to add one of the old URLs using helm repo add unless you supply --allow-deprecated-repos

This is a breaking change, but one that the core maintainers, org maintainers, and chart maintainers have agreed is necessary. Google will turn off the old chart repository on Nov. 13, 2020. This provides warnings to users who are pointing to the old repo, and prevents users from adding the deprecated repos. It stops short of rewriting the repos on the user's behalf.

Special notes for your reviewer:

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@technosophos technosophos added this to the 3.4.0 milestone Oct 15, 2020
@technosophos technosophos self-assigned this Oct 15, 2020
@helm-bot helm-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 15, 2020
cmd/helm/repo_add.go Outdated Show resolved Hide resolved
// parse repo file.
// Ignore the error because it is okay for a repo file to be unparseable at this
// stage. Later checks will trap the error and respond accordingly.
repoFile, err := repo.LoadFile(repofile)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to hit the disk and read this file on every command every time? I'm just thinking out loud. It is a small file but on many commands it will be read more than once.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to alternatives...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid observation from @mattfarina. Can't think of an alternative at moment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, this is blocking the PR merge, but nobody has any alternatives. Not sure what you want me to do.

Copy link
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @technosophos for adding this. Some comments inline.

cmd/helm/repo_add.go Outdated Show resolved Hide resolved
cmd/helm/root.go Outdated Show resolved Hide resolved
// parse repo file.
// Ignore the error because it is okay for a repo file to be unparseable at this
// stage. Later checks will trap the error and respond accordingly.
repoFile, err := repo.LoadFile(repofile)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid observation from @mattfarina. Can't think of an alternative at moment.

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
Copy link
Collaborator

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@technosophos
Copy link
Member Author

Based on a conversation with Matt Farina, we'll merge this one with the pre-check on the repo file. If anyone comes up with a better idea, we can replace.

@technosophos technosophos merged commit af01394 into helm:master Oct 19, 2020
@technosophos technosophos deleted the feat/warn-on-old-repos branch October 19, 2020 20:37
zak905 pushed a commit to zak905/helm that referenced this pull request Jan 19, 2023
Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants