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

Bump and update for fresh chartpressing #1004

Merged
merged 2 commits into from Nov 19, 2019

Conversation

@consideRatio
Copy link
Member

consideRatio commented Nov 18, 2019

I bumped chartpress but also clarified various things etc in docs.

  • removed use of deprecated --commit-range that is now handled
    automatically.
  • trimmed paths in chartpress.yaml to what mattered and documented some
    reasons for that.
  • added notes to the Dockerfile which I spent time understanding more in
    detail while working on this PR.
  • added resetTag and resetVersion to the chartpress.yaml configuration,
    allowing the use of the --reset flag to reset values.yaml and Chart.yaml
    to the state it was in before chartpress was run locally.

Potentially breaking change

Note the Helm chart versions for JupyterHub with names like jupyterhub-0.9.0-alpha.1.033.fd3e470? With chartpress 0.4.3, this is what you will get for BinderHub as well. The pattern is <chartname>-<version>[-<pre-release-tag>[.nnn.sha]], this could influence henchbot etc. perhaps.

charts:
- name: binderhub
imagePrefix: jupyterhub/k8s-
repo:
git: jupyterhub/helm-chart
published: https://jupyterhub.github.io/helm-chart
paths:
- ..

This comment has been minimized.

Copy link
@betatim

betatim Nov 18, 2019

Member

#927 is the PR that added this. Check out the issue linked in it for some details on the bug that was fixed by adding this here. What has changed that we don't need this any more?

This comment has been minimized.

Copy link
@consideRatio

consideRatio Nov 18, 2019

Author Member

Before, chartpress did not consider changes to the images it depended on so this was a hack to resolve it. But, now chartpress does this in this code section, so we don't need this workaround.

This comment has been minimized.

Copy link
@bitnik

bitnik Nov 28, 2019

Collaborator

I think that code section is not in chartpress version 0.4.3, that's must be the reason of #919 (comment)

This comment has been minimized.

Copy link
@consideRatio

consideRatio Nov 28, 2019

Author Member

It was meant to use the info about the file-requirements of the images alongside its own, so it should not had mattered. The issue is that this may still be required but wasnt meant to still be required.

There are tests etc setup in chartpress, but ive apparently missed this test case.

This comment has been minimized.

Copy link
@bitnik

bitnik Nov 28, 2019

Collaborator

Sorry, I think I was not clear or I couldn't understand you.

What I mean is PR jupyterhub/chartpress#68 is not included in v0.4.3. And that PR fixes the problem we have in binderhub right now. I think we need a version upgrade in chartpress or we add the same hack back in chartpress.yaml in binderhub.

# paths are relative to chartpress.yaml
paths:
- ../setup.py
- ../binderhub

This comment has been minimized.

Copy link
@betatim

betatim Nov 18, 2019

Member

I assume these are being proposed for removal because the contextPath already covers them? Is there a way to check that? I have a vague memory that contextPath is the build context sent to dockerd but that the paths are used to determine if an image needs to be built at all.

This comment has been minimized.

Copy link
@consideRatio

consideRatio Nov 18, 2019

Author Member

Correct, but the build context is used as one foundational path alongside those, so these don't add anything as they are part of the build context.

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Nov 18, 2019

@henchc what do you think about the version tag changing and henchbot?

In general I'd be conservative with changing the build infrastructure in this repository. "Never touch a running system." It works well, there are several automations built on top of this that help keep mybinder.org fresh as well as some other public BinderHubs.

Some of the content of the chartpress config were arrived at after debugging subtle bugs that don't always happen (and had gone unnoticed for a long time). So we should take our time when changing things to make sure they don't resurface.

Not against modernisation but at a slow pace please 🐌

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Nov 18, 2019

@choldgraf could you take a look at https://circleci.com/gh/jupyterhub/binderhub/578?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link which seems to be failing because it can't find a tag/reference. Is this a bit of the docs that is being "copy&pasted" from a different repo? It worked a few days ago when master was last built which makes me wonder if the recent changes (pandas theme re-org) in a different repo broke something:-/

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Nov 18, 2019

My guess is that this code is what is now failing @choldgraf. A new PR to fix it would be great.

@consideRatio

This comment has been minimized.

Copy link
Member Author

consideRatio commented Nov 19, 2019

@betatim does this kind of error happen often?

https://travis-ci.org/jupyterhub/binderhub/jobs/613744946?utm_medium=notification&utm_source=github_status

I'm confident it is something I've tracked down and fixed in z2jh + jupyterhub in some PRs, but they require a bump of z2jh + jupyterhub. Anyhow, I assume we can also press restart build job on that to make things succeed.

I bumped chartpress but also clarified various things etc in docs.

- removed use of deprecated --commit-range that is now handled
automatically.
- trimmed paths in chartpress.yaml to what mattered and documented some
reasons for that.
- added notes to the Dockerfile which I spent time understanding more in
detail while working on this PR.
- added resetTag and resetVersion to the chartpress.yaml configuration,
allowing the use of the --reset flag to reset values.yaml and Chart.yaml
to the state it was in before chartpress was run locally.
@consideRatio consideRatio force-pushed the consideRatio:bump-chartpress branch from 1468a89 to 7e32ac0 Nov 19, 2019
@meeseeksmachine

This comment has been minimized.

Copy link

meeseeksmachine commented Nov 19, 2019

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/binder-jupyterhub-activity-round-up-week-1/2704/1

@betatim betatim merged commit e0d720d into jupyterhub:master Nov 19, 2019
4 checks passed
4 checks passed
ci/circleci: build_docs Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing f47294b...7e32ac0
Details
codecov/project 80.12% (target 0%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
yuvipanda pushed a commit to jupyterhub/helm-chart that referenced this pull request Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.