-
Notifications
You must be signed in to change notification settings - Fork 238
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
github: separate workflow for helm repo index update #1359
github: separate workflow for helm repo index update #1359
Conversation
✅ Deploy Preview for kubernetes-sigs-nfd ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: marquiz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/assign @fmuyassarov |
/cherry-pick release-0.13 |
@marquiz: once the present PR merges, I will cherry-pick it on top of release-0.13 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-0.14 |
@marquiz: once the present PR merges, I will cherry-pick it on top of release-0.14 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
f90d485
to
6552732
Compare
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.
Looks much better in my opinion:)
.github/workflows/release.yml
Outdated
- name: Install helm | ||
run: | | ||
curl -sfL https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3 | bash -s -- --version v3.12.3 | ||
|
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.
- name: Install helm | |
run: | | |
curl -sfL https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3 | bash -s -- --version v3.12.3 | |
- name: Install Helm | |
uses: azure/setup-helm@v3 |
I would suggest to use action image here instead which is doing the same thing as curl+bash
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.
Thanks, makes sense, fixed
- name: Push | ||
run: git push -f https://${GITHUB_ACTOR}:${{ secrets.GITHUB_TOKEN }}@github.com/${{ github.repository }} gh-pages |
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.
no strong reasoning, but i found using stefanzweifel/git-auto-commit-action@v4
easier and cleaner to push to remote. But as said, I'm not against having plain git commands on the workflow, just heads up.
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'd like to only rely on "official" actions or at least actions from verified publishers to voting against stefanzweifel. It would look slightly cleaner but doesn't even save any lines of yaml
No need to (re-)build documentation when a release is published. Great simplification of the Helm repo index update script: do not scan all releases but just get the assets from the release that was published. This separation should make the maintenance of scripts and workflows easier.
6552732
to
72bf84c
Compare
release: | ||
types: [published, edited] |
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.
Do you really expect to be editing the releases where there could be some assets added/removed/updated? It is fine perhaps to edit the notes, but assets? Would not that require another patch releases?
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.
Let's say you publish the release but "forget" to add the assets. Then you "edit" the release, adding the missing assets.
/lgtm |
LGTM label has been added. Git tree hash: c94cb43f37d2cbbd5686c266d189904fc1fe3b7d
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1359 +/- ##
=======================================
Coverage 30.00% 30.00%
=======================================
Files 56 56
Lines 7451 7451
=======================================
Hits 2236 2236
Misses 4971 4971
Partials 244 244 |
@marquiz: #1359 failed to apply on top of branch "release-0.13":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@marquiz: #1359 failed to apply on top of branch "release-0.14":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
No need to (re-)build documentation when a release is published. Great simplification of the Helm repo index update script: do not scan all releases but just get the assets from the release that was published.
This separation should make the maintenance of scripts and workflows easier.