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

feat(helm): --no-update flag for helm repo add #1142

Merged
merged 1 commit into from
Oct 18, 2016

Conversation

nicr9
Copy link
Contributor

@nicr9 nicr9 commented Sep 5, 2016

When using --update new repositories will be added in the same way as
without --update but if the repository has already been registered
then the endpoint will be updated.

Closes #1141


This change is Reviewable

@technosophos
Copy link
Member

I'm sometimes opposed to making one subcommand do two different things. But... in this case, I think there is a strong reason why we should do this.

The huge advantage to this is that when the Web UI comes out in a few months, installation instructions will be able to say:

To install this package:
$ helm repo add --update charts https://charts.kubernetes.io
$ helm install foo

That will be a lot simpler for our users than telling them to add a repo if it doesn't already exist, update it if it does exist, and then install the package.

@jackfrancis
Copy link

So I understand the existing behavior before throwing out an opinion...

The current helm repo add command, assuming the repo in question does not already exist, adds the repo, and then performs an update to download locally the currently available collection of charts on the repo. Is this correct?

If that's correct, then I'd argue that helm repo add is already a two-operation action:

  1. add a local repo name-->url mapping
  2. update the local repo with all available remote charts

In which case it's probably more idiomatic to add resiliency to helm repo add in the case of a pre-existing repo name by doing the following:

  1. Verify that the pre-existing repo name maps to the same URL string that the user passed in.
    • for example, if we already have a repo called kremlin that maps to https://charts.kubernetes.ru, and the user passed in the command helm repo add kremlin https://charts.kubernetes.co.uk we should throw an error.
  2. If the pre-existing check above passes, we should print to stdout that "local repo <repo> already exists, updating charts..."

As far as how this ties into the UI, I would suggest that UX/UI design effort includes a prominent, but distinct visual section whose purpose is to communicate the following:

  • This chart requires the "kremlin" repo at https://charts.kubernetes.ru. If you haven't already added this to your local helm installation, run helm repo add kremlin https://charts.kubernetes.ru

As I think about the UI's role in communicating real-world usage of charts that they discover through search/browse, I think there are three distinct categories of info that we want to communicate:

  • prerequisite (i.e., add repo)
  • install options (not sure if there is the notion of different "contexts" to do a helm install foo, but I can imagine there may be eventually)
  • chart customization options (or more specifically, customization recommendations, how-tos, i.e., this chart was purposefully authored to allow useful customizations in this and that way)

The reason I think we should aim for distinguishing between those three categories is because for a large percentage of our users, they will already fulfill the prereq's, and they will not want to customize out of the box; and so by focusing only on "how to install assuming you know what you're doing" we're able to present the simplest, cleanest visual footprint. We don't want to be suggesting to anyone that helm is super complicated, hopefully we can provide the complicated info in a clear way, but in a way that connotes that it's only as complicated as you want it, in most cases it's a cinch!

@technosophos
Copy link
Member

So the tl;dr of @jackfrancis 's post is: we should do the behavior @nicr9 describes as the default.

@nicr9
Copy link
Contributor Author

nicr9 commented Sep 7, 2016

Okay, I'll switch things around so that it'll update by default unless you specify --no-update.

I can see why someone would like to have either behaviour at their disposal.

@nicr9
Copy link
Contributor Author

nicr9 commented Sep 9, 2016

Update: I've not had a lot of time at work to devote to this lately. I've modified the default behaviour but haven't had a chance to update the tests and now I'm out of office on holidays.

I'll finish this as soon as I'm back.

@technosophos
Copy link
Member

Just ping me here or in the Kubernetes Helm channel when you're ready, and I'll review it then.

@nicr9
Copy link
Contributor Author

nicr9 commented Oct 17, 2016

Hey @technosophos,

Sorry it took so long to get back around to this!

Since a lot has happened in cmd/helm/repo.go since this was initially written I've taken the time to scrap my changes and start from scratch!

Just as a reminder: this PR changes the default behaviour for helm repo add so that it updates existing repository entries. I've also added a --no-update flag for those who prefer the old behaviour.

@technosophos technosophos changed the title feat(helm): --update flag for helm repo add feat(helm): --no-update flag for helm repo add Oct 17, 2016
@technosophos
Copy link
Member

Code looks great. I'm going to make sure nobody objects to adding this in the Beta phase, then I will merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants