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

GitHub Actions: Add Release workflow #691

Merged
merged 3 commits into from
Jun 18, 2024

Conversation

ArangoGutierrez
Copy link
Collaborator

No description provided.

@ArangoGutierrez ArangoGutierrez added the testing issue/PR to fix/edit/create/enhance a project unit/e2e test label May 3, 2024
@ArangoGutierrez ArangoGutierrez self-assigned this May 3, 2024
@ArangoGutierrez
Copy link
Collaborator Author

Depends on #687

Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

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

We also spoke about uploading the helm packages and then publishing those to gh-pages. Is that still a goal for this PR?

.github/workflows/release.yaml Outdated Show resolved Hide resolved
hack/update-helm-index.sh Outdated Show resolved Hide resolved
hack/update-helm-index.sh Outdated Show resolved Hide resolved
hack/update-helm-index.sh Outdated Show resolved Hide resolved
hack/update-helm-index.sh Outdated Show resolved Hide resolved
.github/workflows/helm.yaml Outdated Show resolved Hide resolved
.github/workflows/helm.yaml Outdated Show resolved Hide resolved
.github/workflows/helm.yaml Outdated Show resolved Hide resolved
.github/workflows/helm.yaml Outdated Show resolved Hide resolved
.github/workflows/helm.yaml Outdated Show resolved Hide resolved
.github/workflows/helm.yaml Outdated Show resolved Hide resolved
.github/workflows/helm.yaml Outdated Show resolved Hide resolved
hack/update-helm-index.sh Outdated Show resolved Hide resolved
hack/update-helm-index.sh Show resolved Hide resolved
hack/update-helm-index.sh Outdated Show resolved Hide resolved
hack/update-helm-index.sh Outdated Show resolved Hide resolved
hack/update-helm-index.sh Outdated Show resolved Hide resolved
@ArangoGutierrez
Copy link
Collaborator Author

@elezar PTAL

hack/update-helm-index.sh Outdated Show resolved Hide resolved
@ArangoGutierrez ArangoGutierrez force-pushed the releaseAction branch 2 times, most recently from 1f1ce54 to 323a006 Compare May 8, 2024 07:43
Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on this.

Please also update RELEASE.md to reflect the changes here.

hack/update-helm-index.sh Outdated Show resolved Hide resolved
.github/workflows/helm.yaml Outdated Show resolved Hide resolved
.github/workflows/helm.yaml Outdated Show resolved Hide resolved
.github/workflows/helm.yaml Outdated Show resolved Hide resolved
.github/workflows/helm.yaml Outdated Show resolved Hide resolved
hack/update-helm-index.sh Outdated Show resolved Hide resolved
hack/update-helm-index.sh Show resolved Hide resolved
@ArangoGutierrez ArangoGutierrez force-pushed the releaseAction branch 2 times, most recently from 4d6b6e5 to 4e311fa Compare May 8, 2024 09:08
RELEASE.md Outdated Show resolved Hide resolved
hack/update-helm-index.sh Outdated Show resolved Hide resolved
@ArangoGutierrez
Copy link
Collaborator Author

@elezar / @tariq1890 PTAL

Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

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

I think there are some finer points in the implementation that still need to be looked at.

.github/workflows/helm.yaml Outdated Show resolved Hide resolved
Comment on lines +43 to +46
git config user.name "Github Actions"
git config user.email "no-reply@github.com"
Copy link
Member

Choose a reason for hiding this comment

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

Question (not a blocker): can this be set in the action somehow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you mean like as an argument to the action?
The documentation and examples I have found all play it like this.

Comment on lines 35 to 51
GH_EXTRA_ARGS=""
if [[ ${{ github.ref }} == *rc* ]]; then
GH_EXTRA_ARGS="--prerelease"
fi
gh release create ${{ github.ref }} \
--draft \
-t "${{ github.ref }}" \
-n $(./hack/generate-changelog.sh --version ${{ github.ref }}) \
-R $OWNER/$REPO \
--verify-tag \
$GH_EXTRA_ARGS
Copy link
Member

Choose a reason for hiding this comment

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

Is this something that we want to have in a script instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ummm I think this one and the next one(same comment) are good to have here since they are not functionality we would do manually. and is good to have GitHub actions that are readable.
So I would say no, I prefer to keep this as is

Comment on lines 62 to 63
gh release upload ${{ github.ref }} ./nvidia-device-plugin-${{ github.event.release.tag_name }}.tgz -R $OWNER/$REPO
gh release upload ${{ github.ref }} ./gpu-feature-discovery-${{ github.event.release.tag_name }}.tgz -R $OWNER/$REPO
Copy link
Member

Choose a reason for hiding this comment

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

also a script?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

answer on the comment above

RELEASE.md Outdated
- [ ] Wait for the [CI job associated with your tag] (https://gitlab.com/nvidia/kubernetes/device-plugin/-/pipelines) to complete
- [ ] Create a [new release](https://github.com/NVIDIA/k8s-device-plugin/releases) on Github with the changelog
- [ ] Wait for the `Release` GitHub Action to complete
- [ ] Create a [new release](https://github.com/NVIDIA/k8s-device-plugin/releases) from the draft release created by the GitHub Action
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
- [ ] Create a [new release](https://github.com/NVIDIA/k8s-device-plugin/releases) from the draft release created by the GitHub Action
- [ ] Publish the [draft release](https://github.com/NVIDIA/k8s-device-plugin/releases) created by the GitHub Action

Comment on lines 80 to 81
if [ -n "$asset_local" ]; then
echo "Copying $asset_local..."
cp -f $asset_local $HELM_REPO_PATH/stable
fi
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this works the way we want. We are in $HELM_REPO_PATH/stable meaning that $asset_local would be interpreted relative to that path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

HELM_REPO_PATH is the absolute path, so it doesn't matter where we are, the cp will perform the copy to the given path.

Copy link
Member

Choose a reason for hiding this comment

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

What if we have to run these locally as in the "troubleshooting" case?

fi

echo "Updating helm index"
helm repo index stable --merge index.yaml --url https://nvidia.github.io/k8s-device-plugin/stable
Copy link
Member

Choose a reason for hiding this comment

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

We're in $HELM_REPO_PATH/stable. Does that mean that this should be:

```suggestion
helm repo index . --merge index.yaml --url https://nvidia.github.io/k8s-device-plugin/stable


echo "Updating helm index"
helm repo index stable --merge index.yaml --url https://nvidia.github.io/k8s-device-plugin/stable
cp -f stable/index.yaml index.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Since we're in $HELM_REPO_PATH/stable this command is invalid. Should it be:

Suggested change
cp -f stable/index.yaml index.yaml
cp -f index.yaml ../index.yaml

cp -f stable/index.yaml index.yaml
popd > /dev/null

pushd $HELM_REPO_PATH > /dev/null
Copy link
Member

Choose a reason for hiding this comment

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

Should we use git -C for the git commands instead of pushd/popd?


- name: Push updated Helm charts and index to gh-pages branch
env:
HELM_REPO_PATH: releases/helm-${{ github.event.release.tag_name }}/
Copy link
Member

Choose a reason for hiding this comment

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

Question: Could we set HELM_REPO_PATH as a job-level envvar?

@ArangoGutierrez
Copy link
Collaborator Author

Updated as discussed @elezar

hack/update-helm-index.sh Outdated Show resolved Hide resolved
RELEASE.md Outdated Show resolved Hide resolved
RELEASE.md Outdated Show resolved Hide resolved
OWNER: ${{ github.repository_owner }}
REPO: ${{ github.event.repository.name }}
run: |
GH_EXTRA_ARGS=""
Copy link
Member

Choose a reason for hiding this comment

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

This does not work as expected. See https://github.com/NVIDIA/nvidia-container-toolkit/actions/runs/9549456867/job/26319106625

Also when running the gh command line as given, it asks a number of questions. Not sure whether that would be skipped in the action.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed with github.ref_name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should not ask questions, as it should know is running on GItHub Actions mode https://docs.github.com/en/actions/using-workflows/using-github-cli-in-workflows

OWNER: ${{ github.repository_owner }}
REPO: ${{ github.event.repository.name }}
run: |
gh release upload ${{ github.ref_name }} ./nvidia-device-plugin-${{ github.event.release.tag_name }}.tgz -R $OWNER/$REPO
Copy link
Member

Choose a reason for hiding this comment

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

What value does tag_name have? Do we strip the leading v?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

responds to refs/tags/<tag_name> so it will include the v

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Our packages don't include the v though. Can we update this?

Also, as a matter of interest, why do we use tag_name instead of just using ref_name consistently?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how about now?

ArangoGutierrez and others added 3 commits June 18, 2024 14:37
This change updates the release scripting to create helm packages
and update the helm index in the gh-pages branch.

Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
This change adds GitHub actions that create a draft release when a
tag is pushed. This draft release incldes Helm packages for the
device plugin.

Once this release is published, these helm packages are published to
the gh-pages branch.

Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

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

I have made the required changes to the branch.

$GH_EXTRA_ARGS

- name: Install Helm
uses: azure/setup-helm@v4.2.0
Copy link
Member

Choose a reason for hiding this comment

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

Question: Is this one of the allowed actions?

- name: Install Helm
uses: azure/setup-helm@v4.2.0
with:
version: 3.14.4
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to just use the latest stable version here? Not a blocker for this PR though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am never against latest during testing, so I agree we should go latest

@@ -50,7 +50,7 @@ while [[ $# -gt 0 ]]; do
shift # past value
;;
--version)
VERSION="$2"
VERSION="$2#v"
Copy link
Member

Choose a reason for hiding this comment

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

This wasn't required in this script.

@elezar elezar enabled auto-merge June 18, 2024 12:39
@elezar elezar merged commit ad4aae5 into NVIDIA:main Jun 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing issue/PR to fix/edit/create/enhance a project unit/e2e test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants