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

helm chart binderhub-0.2.0-409f200 contains bhub image with tag 0.2.0-9174211 #919

Closed
bitnik opened this issue Aug 8, 2019 · 15 comments
Closed

Comments

@bitnik
Copy link
Collaborator

@bitnik bitnik commented Aug 8, 2019

Hi, I just deployed binderhub-0.2.0-409f200 on GESIS Binder, but I realised that it uses binderhub image with tag 0.2.0-9174211 instead of 0.2.0-409f200. I think something went wrong when #838 is merged or am I wrong? Actually it is only documentation update. So it shouldn't create a new chart actually?

If you point me the code where chart generation is done, I can also look at what is wrong.

@betatim

This comment has been minimized.

Copy link
Member

@betatim betatim commented Aug 8, 2019

We use https://github.com/jupyterhub/chartpress to make the charts and it is called in

echo "building helm chart"
For some runs $PUSH contains the arguments that ask chartpress to publish stuff.

@bitnik

This comment has been minimized.

Copy link
Collaborator Author

@bitnik bitnik commented Aug 8, 2019

I just ran chartpress locally

git checkout 9174211
chartpress

Output

$> git log -n 1 --pretty=format:%h -- . ../setup.py ../binderhub
$> git log -n 1 --pretty=format:%h -- images/image-cleaner chartpress.yaml
Skipping build for jupyterhub/k8s-image-cleaner:0.2.0-c56fb9e, it already exists
$> git log -n 1 --pretty=format:%h -- ../setup.py ../binderhub .. chartpress.yaml
Skipping build for jupyterhub/k8s-binderhub:0.2.0-9174211, it already exists
Updating binderhub/values.yaml: imageCleaner.image: {'repository': 'jupyterhub/k8s-image-cleaner', 'tag': '0.2.0-c56fb9e'}
Updating binderhub/values.yaml: image: {'repository': 'jupyterhub/k8s-binderhub', 'tag': '0.2.0-9174211'}

For helm chart tag it runs git log -n 1 --pretty=format:%h -- . ../setup.py ../binderhub which outputs 409f200 on commit 9174211 because last change is done in doc folder. But for image tag it runs git log -n 1 --pretty=format:%h -- ../setup.py ../binderhub .. chartpress.yaml with extra paths .. chartpress.yaml and it outputs 9174211. That is the reason why image and helm chart tags are different. Those extra paths are added in `build_imagesmethod (https://github.com/jupyterhub/chartpress/blob/master/chartpress.py#L221-L224).

@bitnik

This comment has been minimized.

Copy link
Collaborator Author

@bitnik bitnik commented Aug 8, 2019

This causes that 2 commits 409f200 and 9174211 are creating helm charts with same version but each chart has different image. And second chart overrides the first one.

@betatim

This comment has been minimized.

Copy link
Member

@betatim betatim commented Aug 8, 2019

Not a chartpress expert but sounds like we should use the same set of paths to decide if a new chart tag is needed and if a new image is needed. Naively the chart tag determination should be the OR of all image tag determinations: if anything is updated then the chart tag also has to be updated?

@bitnik

This comment has been minimized.

Copy link
Collaborator Author

@bitnik bitnik commented Aug 12, 2019

I am not chartpress expert neither but I think the same. Image and chart should have same tags.

Should I move this issue to chartpress repo?

@betatim

This comment has been minimized.

Copy link
Member

@betatim betatim commented Aug 12, 2019

Sounds good. Or make a PR straight away? I think Min is the only "true" chartpress expert :)

@chicocvenancio

This comment has been minimized.

Copy link
Contributor

@chicocvenancio chicocvenancio commented Aug 12, 2019

I fail to see the problem with different image and chart tags, to me it seems expected that they won't match at times.

@bitnik

This comment has been minimized.

Copy link
Collaborator Author

@bitnik bitnik commented Aug 13, 2019

@minrk what do you think?

Before I wanted to move this issue to chartpress repo because I thought there is a problem on chartpress side. But now I see that actually it takes all those paths from chartpress.yaml and also there is contextPath specified for binderhub image:

charts:
- name: binderhub
imagePrefix: jupyterhub/k8s-
repo:
git: jupyterhub/helm-chart
published: https://jupyterhub.github.io/helm-chart
paths:
- ../setup.py
- ../binderhub
images:
image-cleaner:
valuesPath: imageCleaner.image
binderhub:
# Context to send to docker build for use by the Dockerfile
contextPath: ..
# Dockerfile path relative to chartpress.yaml
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

Maybe we should just add path .. into charts.paths?

@betatim

This comment has been minimized.

Copy link
Member

@betatim betatim commented Aug 13, 2019

I think the problem isn't that sometimes the chart and image tag don't match but that we produce the same chart tag but with two different image tags in it.

At moment A create a new chart with tag x containing image tag a1.

At moment B (later in time) create a new chart (also) with tag x but now containing image tag b1. The chart tag should be y because its content changed.

That is how I read the bug report. I think the fact that we produce two charts with the same tag but different "content" is a bug.

@betatim

This comment has been minimized.

Copy link
Member

@betatim betatim commented Aug 13, 2019

Maybe we should just add path .. into charts.paths?

I think we should try this and see what happens. Can always later make a PR to chartpress that it automatically considers the union of charts.paths and the paths and contextPath of all the images.

@bitnik

This comment has been minimized.

Copy link
Collaborator Author

@bitnik bitnik commented Sep 3, 2019

Closing this issue. I think this is solved by #927. We can re-open this issue if it happens again.

@bitnik bitnik closed this Sep 3, 2019
@bitnik bitnik reopened this Nov 28, 2019
@bitnik

This comment has been minimized.

Copy link
Collaborator Author

@bitnik bitnik commented Nov 28, 2019

This happened again. PR #1008 made only changes in doc folder. And chartpress created same chart version (binderhub-0.1.0-456.7e32ac0) with a new image (jupyterhub/k8s-binderhub:0.1.0-459.ede498d).

Chart binderhub-0.1.0-456.7e32ac0 was already created after PR #1004 with image
jupyterhub/k8s-binderhub:0.1.0-456.7e32ac0 and this is actually what is deployed on mybinder.org and GESIS Binder. Yesterday (after #1008) I was expecting a new PR to upgrade binderhub, but it didn't happened and this is how I realised the problem.

I don't have time to really investigate this but I think this is caused by #1004.

@consideRatio

This comment has been minimized.

Copy link
Member

@consideRatio consideRatio commented Nov 28, 2019

Oh! Chartpress should always produce a more modern chart version than chart image =/ that is a bug. I bet it relates to the contextPath of the binderhub image being broader than the chart's context itself somehow.

@bitnik

This comment has been minimized.

Copy link
Collaborator Author

@bitnik bitnik commented Nov 28, 2019

I just looked at #1004 in more detail (and made a comment there). I think this is fixed in chartpress but the version we use here doesn't include that fix.

@bitnik

This comment has been minimized.

Copy link
Collaborator Author

@bitnik bitnik commented Nov 28, 2019

Closing again by #1014

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.