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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,5 +6,5 @@ pytest-asyncio | |
pytest-cov | ||
requests | ||
ruamel.yaml>=0.15 | ||
chartpress==0.3.1 | ||
chartpress>=0.4.3,<0.5 | ||
jupyterhub |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,23 +1,28 @@ | ||
# For a reference on this configuration, see the chartpress README file. | ||
# ref: https://github.com/jupyterhub/chartpress | ||
charts: | ||
- name: binderhub | ||
imagePrefix: jupyterhub/k8s- | ||
repo: | ||
git: jupyterhub/helm-chart | ||
published: https://jupyterhub.github.io/helm-chart | ||
paths: | ||
- .. | ||
resetTag: local | ||
resetVersion: 0.2.0 | ||
# NOTE: All paths will be set relative to this file's location, which is in the | ||
# helm-chart folder. | ||
images: | ||
image-cleaner: | ||
valuesPath: imageCleaner.image | ||
binderhub: | ||
# Context to send to docker build for use by the Dockerfile | ||
# Context to send to docker build for use by the Dockerfile. We pass the | ||
# root folder in order to allow the image to access and build the python | ||
# package. Since we do that, chartpress will always react to changes in | ||
# documentation and other things, and always consider the chart version | ||
# to change along with the image version. | ||
contextPath: .. | ||
# Dockerfile path relative to chartpress.yaml | ||
# Since we changed the contextPath, we must also change the | ||
# dockerfilePath. This is because chartpress assume the Dockerfile will | ||
# reside in the contextPath folder, and since we overrode the default of | ||
# images/binderhub it will be the wrong folder. | ||
dockerfilePath: images/binderhub/Dockerfile | ||
valuesPath: image | ||
# additional paths relevant to the image | ||
# in addition to image directory itself | ||
# paths are relative to chartpress.yaml | ||
paths: | ||
- ../setup.py | ||
- ../binderhub | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume these are being proposed for removal because the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
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.
#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?
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.
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.
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 think that code section is not in chartpress version 0.4.3, that's must be the reason of #919 (comment)
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.
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.
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.
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.