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

Include previous helm chart in helm index #1183

Merged
merged 2 commits into from
Jan 18, 2023

Conversation

miles-w-3
Copy link
Contributor

SUMMARY

Fixes #1136 - index.yaml will be fully generated on each release

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
ADDITIONAL INFORMATION

There is still some testing that needs to be done on the master branch to ensure that the remote is configured properly (i don't think a new remote needs to be added since we aren't using cr right now)

There are cleaner ways to do this, either with being smarter about not re-generating the index with every tag, or with using CR, but this should at least guarantee that the index can point to the proper release links, which fixes the immediate problem.

# package the current release into the .cr-release-packages dir (phony for helm-chart target as well)
make helm-package
# download all previously tagged releases into .cr-release-packages dir, switch branch to gh-pages, update the index.yaml, and push
make helm-index

Makefile Outdated
# checkout gh-pages branch and push up the new index file
git switch gh-pages
# TODO: Assess removal of additional origin
git fetch --all --tags
Copy link
Member

Choose a reason for hiding this comment

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

With --all, all remotes will be fetched. Is that intended? Do you maybe want git fetch origin --tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think that would be fine, i was having trouble getting all of the tags on my fork, I can remove --all as well as the TODO

@miles-w-3
Copy link
Contributor Author

@rooftopcellist I just want to point out, the promote pipeline is set to manual trigger for testing right now, but we probably don't want it that way when merged

@janorn
Copy link
Contributor

janorn commented Jan 13, 2023

@miles-w-3 thanks for helping out on this. I was stuck.
Just a note while my testing i noticed the cr actually created .cr-release-packages when creating the release.

@miles-w-3
Copy link
Contributor Author

Do you mean that it created the dir or the files inside? My problem with cr is that when I ran it, it said "nothing new from previous releases", or something like that, my guess is because the releases aren't committed to git

@janorn
Copy link
Contributor

janorn commented Jan 16, 2023

When I setup the make on my localhost running cr it created exactly that dir you created with mkdir when creating the release. Next step was to download all the older releases. However the issue I ran into was that the git checkout is shallow and does not contain all tags. I tried to work around that but that didn't seem to work. I blame that on my limited Makefile experience 😄

@janorn
Copy link
Contributor

janorn commented Jan 16, 2023

But saying that. It is never wrong to be explicit. So keep the mkdir!

Makefile Outdated
cd .cr-release-packages;\
for tag in $(TAGS); do\
dl_url="https://github.com/$${CHART_OWNER}/$${CHART_REPO}/releases/download/$${tag}/$${CHART_REPO}-$${tag}.tgz";\
curl -RLOs -z "$${CHART_REPO}-$${tag}.tgz" --fail $${dl_url};\
dl_url="https://github.com/$${CHART_OWNER}/$${CHART_REPO}/releases/download/$${tag}/$${CHART_REPO}-$${tag}.tgz";\ curl -RLOs -z "$${CHART_REPO}-$${tag}.tgz" --fail $${dl_url};\
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you lost a linefeed here.

@miles-w-3
Copy link
Contributor Author

I manually added the dir since this solution is currently just using the helm repo index command instead of cr, since I ran into other issues with cr

@janorn
Copy link
Contributor

janorn commented Jan 17, 2023

Missed that 🤣

@rooftopcellist
Copy link
Member

@miles-w-3 @TheRealHaoLiu and I walked through this PR and a full fake release on my fork today. There were a couple changes needed for it to work. The main issue we ran into is that in the make helm-index target, when it switches to the gh-pages branch, the make command "short-circuits" for lack of a better way of saying it. Because the Makefile and ansible/ directory don't exist on the gh-pages branch, it fails early right after switching branches.

Having a separate Makefile and ansible/ in the gh-pages branch that would need to also be maintained isn't the best solution, but we were able to find a good way to make it work by cloning the awx-operator again, checking out the gh-pages branch, and moving in to that directory.

All of this and a couple other small fixes are in this commit, you should be able to just cherry-pick it if it looks good to you:

- checkout gh-pages branch in promote.yaml
- fix helm-index make target
- add gh-pages folder in .gitignore

Co-Authored-By: Christian Adams <[email protected]>
@rooftopcellist
Copy link
Member

Thanks for all the hard work @miles-w-3 and @janorn !

@rooftopcellist rooftopcellist merged commit f6f58d5 into ansible:devel Jan 18, 2023
@janorn
Copy link
Contributor

janorn commented Jan 24, 2023

Something didn't work. No helm index for release 1.1.4

@miles-w-3
Copy link
Contributor Author

Yep, the index yaml on the branch didn't change. I'm on mobile, can you see the status of the promote pipeline?

@janorn
Copy link
Contributor

janorn commented Jan 25, 2023

Sorry late here and I'm also on mobile. Can try and have a look in the morning.

@rooftopcellist
Copy link
Member

rooftopcellist commented Jan 25, 2023

@miles-w-3 @janorn It looks like what's causing the error is a type in the Release tarball URL generation. So it reports that there is "No helm chart present" and skips.

https://github.com/ansible/awx-operator/releases/download/0.7.0/awx-operator-0.7.0.tgz

Odd that this worked when we tested it on my fork, the typo must have gotten in there before pushing somehow..

"Skipping release 0.14.0; No helm chart present"

Nevermind, that's not the failure, those versions were just from before we had a helm chart. I'll keep digging.

@rooftopcellist
Copy link
Member

@miles-w-3 @janorn I put a fix up in #1199 that fixes the issue. The tar.gz's weren't ending up in the .cr-release-packages/ directory because of a change @TheRealHaoLiu and I added to get the gh-pages directory (rather than branch) approach to work.

I ran the make target locally to push the updated index.yaml file.

$ CHART_OWNER=ansible make helm-index

https://github.com/ansible/awx-operator/blob/gh-pages/index.yaml

@TheRealHaoLiu TheRealHaoLiu changed the title Setup make index for testing Include previous helm chart in helm index Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Previous helm chart versions not available
4 participants