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

use curl instead of cr #289

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

use curl instead of cr #289

wants to merge 1 commit into from

Conversation

pat-s
Copy link
Collaborator

@pat-s pat-s commented Jan 9, 2025

More issues than help. Broke again after the most recent 1.7 release when releasing chart 2.0.4.

A curl command to upload to create a GH release directly should be more resilient.

@pat-s pat-s added the build label Jan 9, 2025
@pat-s pat-s requested a review from xoxys January 9, 2025 23:27
@xoxys
Copy link
Member

xoxys commented Jan 10, 2025

Compared to cr its really unreadable 🙈 whats the current issue with the latest cr release? Just stick with the working version for now if its a known bug?

@pat-s
Copy link
Collaborator Author

pat-s commented Jan 10, 2025

Well, cr is also not really readable (why does it need like five flags to do a simple gh-pages publish?) and in the end it is only doing an API call to the GH releases endpoint anyway.

Personally, I'd drop the gh-pages upload entirely.
I am absolutely unhappy maintaining cr solely for this purpose. In all environments/projects I've used in the last 3 years it broke for almost every point release (and in between). In addition, I don't see why one would need an additional packaging wrapper just for helm charts. The OCI variant shows how easy a chart release can be and is on top using helm itself.

So: either somebody else commits to maintain cr from here onward or we drop it. Overall I'd prefer dropping the upload to gh-pages entirely and just keep the OCI variant (and yes I know this is breaking).

@@ -7,26 +7,21 @@ when:

steps:
pack-chart:
image: quay.io/helmpack/chart-releaser:v1.7.0
image: alpine/helm:3.16.4
Copy link
Member

Choose a reason for hiding this comment

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

This is not an official image from the alpine maintainers. Dont know how trustworthy the author is.


release-chart:
image: quay.io/helmpack/chart-releaser:v1.7.0
image: alpine:3.21
Copy link
Member

Choose a reason for hiding this comment

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

Should be full qualified container name.

@xoxys
Copy link
Member

xoxys commented Jan 10, 2025

Fiddling around with jq curl and json input is also error prone and Im using cr across multiple repos without such issues. But at the end I dont care much, if you think this process is more stable.

@xoxys
Copy link
Member

xoxys commented Jan 13, 2025

@pat-s I see you reverted cr. If you addressed the open comments here, we could also merge this PR.

@pat-s
Copy link
Collaborator Author

pat-s commented Jan 14, 2025

To get the release published successfully. I couldn't ensure that the release will work with this PR as I didn't test it on a test repo explicitly.
I didn't want the release to be postponed, which is why I opened #296.

I'll implement a related process in a new repo with write access soon and it seemed overall safer to me to proceed this way with the latest release.

I'll leave it up to you to continue here/decide what to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants