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

Pre-release replaced by later builds #62

Closed
consideRatio opened this issue Oct 19, 2019 · 2 comments · Fixed by #64
Closed

Pre-release replaced by later builds #62

consideRatio opened this issue Oct 19, 2019 · 2 comments · Fixed by #64
Labels
bug Something isn't working

Comments

@consideRatio
Copy link
Member

consideRatio commented Oct 19, 2019

I've found a reproducible issue since #52 relating to the use of chartpress. Apparently, if we use chartpress to publish a tagged commit, like 0.9.0-alpha.1, and later use chartpress to publish a later commit, it appears that Helm chart repository's index.yaml will get its entry about 0.9.0-alpha.1 replaced by 0.9.0-alpha.1+001.g152b8c9a, instead of having it added alongside...

jupyterhub/helm-chart@29f7cbe#diff-43a27642790006c93fea79f384f23b4fL2858-R2859

Reproduction

You need chartpress and helm, where helm is initialized with helm init --client-only.

pip install chartpress=0.4.2
rm -rf /tmp/{z2jh,helm-chart,index}
mkdir /tmp/{z2jh,helm-chart,index}

pushd /tmp/helm-chart
git clone https://github.com/jupyterhub/helm-chart . --no-checkout 
git checkout 6c4de08
popd

pushd /tmp/index
git init
helm repo index . --merge /tmp/helm-chart/index.yaml --url https://jupyterhub.github.io/helm-chart
git add index.yaml
git commit -m "index.yaml initial state: with 0.9.0-alpha.1"
rm --interactive=never /tmp/index/*
popd



pushd /tmp/z2jh
git clone https://github.com/jupyterhub/zero-to-jupyterhub-k8s . --no-checkout
git checkout 83af061d
chartpress --skip-build
helm package ./jupyterhub --destination /tmp/index
mv /tmp/index/jupyterhub-0.9.0-alpha.1+001.g152b8c9.tgz /tmp/index/jupyterhub-0.9.0-alpha.1_001.g152b8c9.tgz
popd

pushd /tmp/index
helm repo index . --merge /tmp/helm-chart/index.yaml --url https://jupyterhub.github.io/helm-chart
git add index.yaml
git commit -m "merge with 0.9.0-alpha.1+n.sha overrode 0.9.0-alpha.1 entry"
cp /tmp/index/* /tmp/helm-chart/
rm --interactive=never /tmp/index/*
#rm /tmp/index/index.yaml
popd



pushd /tmp/z2jh
git checkout 0.9.0-alpha.1
chartpress --skip-build
helm package ./jupyterhub --destination /tmp/index
popd

pushd /tmp/index
helm repo index . --merge /tmp/helm-chart/index.yaml --url https://jupyterhub.github.io/helm-chart
rm --interactive=never /tmp/index/jupyterhub-*
git add index.yaml
git commit -m "Should not do anything."

Reproduction results

   - apiVersion: v1
     appVersion: 1.0.1dev
-    created: "2019-10-17T22:17:04.353205591Z"
+    created: "2019-10-19T20:01:39.379225412+02:00"
     description: Multi-user Jupyter installation
-    digest: 4835bf4b9d3130ad5747052a0eec50f1e5b2ef5133b9084d3a4e5a9f3602cc3e
+    digest: 9789053b0136b06fe3be4e429c1b57d69cc097faac80cdaf21239afa36f650a7
     home: https://z2jh.jupyter.org
     icon: https://jupyter.org/assets/hublogo.svg
     kubeVersion: '>=1.11.0-0'
@@ -2847,8 +2847,8 @@ entries:
     - https://github.com/jupyterhub/zero-to-jupyterhub-k8s
     tillerVersion: '>=2.11.0-0'
     urls:
-    - https://jupyterhub.github.io/helm-chart/jupyterhub-0.9.0-alpha.1.tgz
-    version: 0.9.0-alpha.1
+    - https://jupyterhub.github.io/helm-chart/jupyterhub-0.9.0-alpha.1+001.g152b8c9.tgz
+    version: 0.9.0-alpha.1+001.g152b8c9
@consideRatio
Copy link
Member Author

consideRatio commented Oct 19, 2019

Confusion points

"g" was added to the commit hash

Aha, apparently the git describe command adds a "g" prefix. Hmm... https://git-scm.com/docs/git-describe#_examples

Specifying filename when running helm package

Apparently we cannot specify a filename for the helm package, but the filename doesn't really matter when an index.yaml file is created. It will simply use the Chart.yaml version, but that contains a plus sign sometimes, and becomes an invalid Windows filename. I think it is actually a valid filename on windows 10. Okay!

helm repo index --merge

Hmmm... If the name and version does not exist, we should have it become appended. It isn't. This is what I think leads to the observed outcomes.

Merge function -> Has function -> Get function

I think the Get function returns non-errorish, and then Has returns true, and Merge decides to not append the chart entry in the merge operation.

I suspect this section leads to the return that should not happen.

	for _, ver := range vs {
		test, err := semver.NewVersion(ver.Version)
		if err != nil {
			continue
		}

		if constraint.Check(test) {
			return ver, nil
		}
	}

datetime quoting

What is going on here with all the quoting?

jupyterhub/helm-chart@29f7cbe#diff-43a27642790006c93fea79f384f23b4fL2858-R2859

@consideRatio
Copy link
Member Author

consideRatio commented Oct 21, 2019

I've debugged this in depth and concluded that Helm has either a bug or to me very unexpected behavior, where only one build will be found in the index.

Option 1 : Abandon + entirely in favor of .

Pro: we can now trust helm to work the way we want. I think I have verified this in the opened helm issue.
Pro: we can do this quickly, and it does not require any wait
Pro: we have already tried using many semver 2 pre-releases as dev-builds, as 0.9-asdf1234 is one for example, and that is a typical chart version for z2jh for example.
Pro: we can align the image version exactly with the chart version, as they don't contain different character sets.
?: We abandon the use of + in favor of .

Option 2: Workaround the shortcoming of helm repo index --merge specifically.

Pro: We can keep using + as I've been thinking make sense to do.
?: We retain the use of +.

Option 3: Await upstream helm fixes

Con: It takes too long time, chartpress is in a very bad state now due to this issue.

Considerations

  • We need to resolve this fast as 0.4.2 is not working good enough for our typical use of it in BinderHub and Z2JH, probably not pangeo's helm-chart either, by replacing old index entries with new if they have the same tag but differing build suffix.
  • Going from pre Chart and image versioning, and Chart.yaml's --reset interaction #52 versioning system to this where we use . instead of + for build suffix separator is probably fine.

Yeah. I'm think we should go for Option 2.

Implementation notes

To make the transition from + to ., we need to consider some things.

  • previously only had one of the build separator signs (+), but now we will use one that is already in use both in the x.y.z versioning, and potentially in the latest tag's pre-release section.

  • we need to start out by appending either a - or . depending on if the latest tag is a release or a pre-release, because having two - separators would make it an invalid SemVer 2 version. It should look something like this.

    NOTE: the .dirty part is only a mental preparation for implementing PR Discussion - Manage uncommitted changes #49

    0.8.0
    0.8.0-004.asdf123
    0.8.0-010.sdfg234
    0.9.0-beta.1
    0.9.0-beta.1.001.dfgh345
    0.9.0-beta.1.005.fghj456
    0.9.0-beta.2
    0.9.0-beta.2.001.ghjk567
    0.9.0-beta.2.001.ghjk567.dirty
    0.9.0-beta.3
    0.9.0
    0.9.0-dirty
    

@consideRatio consideRatio added the bug Something isn't working label Oct 21, 2019
@minrk minrk closed this as completed in #64 Oct 22, 2019
consideRatio added a commit to jupyterhub/helm-chart that referenced this issue Nov 9, 2019
By cleaning these up, I can make a better info.json template file that
can be used with badges to give us the latest tags of relevance etc. My
goal is to make a tag that shows the latest stable, latest pre-release,
and latest dev release.

Related issue that caused the initial use of + sign to be used in
versions followed by us switching away from it later:
jupyterhub/chartpress#62
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant