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

Add --force-publish-chart and default to not overwriting #102

Merged

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Nov 14, 2020

This feature help us avoid publishing the same chart version twice unless --force-publish-chart is passed. This mirrors the --push and --force-push behavior of docker images.

There are several reasons to not push an already existing chart:

  1. It can trigger other pointless actions, such as henchbot suggesting mybinder.org-deploy to update.
  2. It will clutter the gh-pages branch of the helm-chart git repository we push to. This clutter is both visual for any user browsing the versions and makes the helm chart repo grow bigger and bigger.

Closes #60 (the need for this kind of feature)
Closes #73 (implementation planning)
Closes #75 (an old PR to do this pretty much)

Review notes

This is the relevant implementation changing the default behavior to check unless --force-publish-chart has been passed.

https://github.com/jupyterhub/chartpress/pull/102/files#diff-b221c94b491bd2ffe6ebc5416067fd58b378f4eda722e75754468ed9f24aefefR587-R601

@consideRatio consideRatio added the enhancement New feature or request label Nov 14, 2020
README.md Show resolved Hide resolved
chartpress.py Outdated Show resolved Hide resolved
chartpress.py Show resolved Hide resolved
chartpress.py Show resolved Hide resolved
chartpress.py Outdated Show resolved Hide resolved
@betatim
Copy link
Member

betatim commented Nov 16, 2020

Can you add a test for the "with force" case as well?

@consideRatio
Copy link
Member Author

consideRatio commented Nov 16, 2020

Thank you for your review @betatim ! ❤️ 🎉 Everything addressed!

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

My only thought is whether attempting to overwrite an existing version chart should return an error code instead of being silently ignored, in the same way that publishing the same version of a Pypi or Conda package will fail by default?

OTOH the current behaviour of always overwriting is different from those package managers, so maybe it doesn't matter.

@consideRatio
Copy link
Member Author

My only thought is whether attempting to overwrite an existing version chart should return an error code instead of being silently ignored, in the same way that publishing the same version of a Pypi or Conda package will fail by default?

While it is printing a message about either overwriting when force is applied and skipping publication if force isn't applied, I agree it is almost silent.

I think I like it as it is, if we would start failing, it would be a very breaking change that can force people to default to --force-publish-chart to not have such failures, because it would be wrong to do chartpress ... || true or similar as that can fail for another reason that you don't want to ignore by || true. So I figure if we would let it fail, we need yet another option.

I think skipping is the most sensible default, and it would be the most logical default matching how we do with --push / --force-push also, even though a docker push defaults to overwriting the image tag.

Yeah having considered it a bit now I think the current PR implementation reflects the desired behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants