-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: use noctua in our CI #207
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you test this in a few of our repos first and link examples of it running? Its hard to detect if it actually works just from the code, and hard to test as a reviewer Without fully reading the PR yet, I'd wonder about things like the metadata generated for uploads/releases and if we're making something equivalent here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you give an example of what metadata you're referring to? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
lucabello marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,12 +56,21 @@ jobs: | |
with: | ||
fetch-depth: 1 | ||
- name: Check charm libraries # Make sure our charm libraries are updated | ||
uses: canonical/charming-actions/[email protected] | ||
with: | ||
credentials: "${{ secrets.CHARMHUB_TOKEN }}" | ||
github-token: "${{ secrets.GITHUB_TOKEN }}" | ||
env: | ||
CHARMHUB_TOKEN: "${{ secrets.CHARMHUB_TOKEN }}" | ||
GITHUB_TOKEN: "${{ secrets.GITHUB_TOKEN }}" | ||
charm-path: "${{ inputs.charm-path }}" | ||
charmcraft-channel: "${{ inputs.charmcraft-channel }}" | ||
run: | | ||
pip install git+https://github.com/lucabello/noctua | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add tags in this target repo and use them here? That way we can make changes to the package without them always going live here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I will add this :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prior to merging this, can we have a CI set up, similar to what we have in cos-lib, so that noctua is auto tagged on release? |
||
outdated_libs=$(noctua charm libraries check --minor | jq 'length') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A somewhat big ask, that we could leave for a future task:
|
||
if [[ $outdated_libs != "0" ]]; then | ||
gh pr edit ${{ github.event.number }} --remove-label="Libraries: OK" | ||
gh pr edit ${{ github.event.number }} --add-label="Libraries: Out of sync" | ||
else | ||
gh pr edit ${{ github.event.number }} --remove-label="Libraries: Out of sync" | ||
gh pr edit ${{ github.event.number }} --add-label="Libraries: OK" | ||
fi | ||
static-analysis: | ||
name: Static Analysis | ||
uses: canonical/observability/.github/workflows/_charm-static-analysis.yaml@main | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,12 @@ on: | |
required: false | ||
description: | | ||
The snap channel from which to install Charmcraft. | ||
release-channel: | ||
type: string | ||
default: latest/edge | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are we sure about this default channel? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are; we currently release stuff to |
||
required: false | ||
description: | | ||
The default channel to release charms to. | ||
secrets: | ||
CHARMHUB_TOKEN: | ||
required: true | ||
|
@@ -136,27 +142,22 @@ jobs: | |
uses: actions/checkout@v4 | ||
with: | ||
fetch-depth: 1 | ||
- name: Select charmhub channel | ||
uses: canonical/charming-actions/[email protected] | ||
id: channel | ||
- name: Fetch charm artifacts | ||
uses: actions/download-artifact@v3 | ||
with: | ||
name: charms | ||
path: "${{ github.workspace }}/${{ inputs.charm-path }}" | ||
- name: Upload charm to charmhub | ||
uses: canonical/charming-actions/[email protected] | ||
with: | ||
credentials: "${{ secrets.CHARMHUB_TOKEN }}" | ||
github-token: "${{ secrets.GITHUB_TOKEN }}" | ||
channel: "${{ steps.channel.outputs.name }}" | ||
built-charm-path: "${{ matrix.path }}" | ||
charm-path: "${{ inputs.charm-path }}" | ||
tag-prefix: "${{ inputs.release-tag-prefix }}" | ||
charmcraft-channel: "${{ inputs.charmcraft-channel }}" | ||
# We set destructive mode to false, otherwise runner's OS would have to match | ||
# charm's 'build-on' OS. | ||
destructive-mode: false | ||
- name: Upload charm to Charmhub | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you considered packaging this not as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, and testing. makes testing possible without needing the whole system There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have, but I disagree on the benefits. I think being able to see the logic is a big advantage: no one in the team knows exactly what charming-actions do for this exact reason. We already have re-usable workflows that abstract the work: this very file is not even used directly, so if you want a high-level view there's I'd also argue that looking at a few lines of bash is a lot simpler than having another level of nesting ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. About testing, it's true you can't unit-test these actions as a whole. I'd argue that for that, I'd rather have a fake repository where we run these workflows live, and we test new changes there instead. The
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Re using composite actions, agreed it is subjective. I like the benefits of:
and I don't mind paying for that with an extra layer of abstraction. But there's no right or wrong There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as for testing, we should have something more than our production repos being the first recorded result. There's options to make that work (this repo could trigger a PR on another repo, or at least we run this branch through a few repos and link them in the PR description) |
||
env: | ||
CHARMHUB_TOKEN: ${{ secrets.CHARMHUB_TOKEN }} | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
run: | | ||
pip install git+https://github.com/lucabello/noctua | ||
cd "${{ inputs.charm-path }}" | ||
release=$(noctua charm release --path="${{ matrix.path }}" --channel=${{ inputs.release-channel }} --json | tail -n1) | ||
lucabello marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use something less fragile than |
||
revision=$(echo "$release" | jq .revision) | ||
# TODO: push git tag; is it necessary? | ||
gh release create "${{inputs.release-tag-prefix}}rev${revision}" --title="Revision ${revision}" --generate-notes | ||
Comment on lines
+156
to
+160
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. more a question than suggestion, but where do you see the line for what should/shouldn't be in the noctua tool? I wonder if the If noctua stayed as-is but the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A problem with adding that to I'm trying to follow the "path of least surprises", trying to stay closer to I'm not sure if I understand what's the benefit of making a composite action for this, considering it's one line of Bash; it would, as you said, hide the logic, and I don't see a difference in readability between an action saying ("create charm release on github") and a comment just above that line with the same message. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this goes in the same direction as this thread so we can probably discuss it there. Its subjective, but I like the cleanliness like you have in the public workflows - they're very easy for someone else to read and understand the intent. As soon as run commands get involved, the clarity for a reader who doesn't want the details goes down |
||
release-arm-to-charmhub: | ||
name: Release arm64 to CharmHub | ||
# needs to be run on arm or the oci image will resolve to the amd64 one. | ||
|
@@ -172,31 +173,19 @@ jobs: | |
uses: actions/checkout@v4 | ||
with: | ||
fetch-depth: 1 | ||
- name: Select charmhub channel | ||
uses: canonical/charming-actions/[email protected] | ||
id: channel | ||
- name: Fetch charm artifacts | ||
uses: actions/download-artifact@v3 | ||
with: | ||
name: charms-arm | ||
path: "${{ github.workspace }}/${{ inputs.charm-path }}" | ||
- name: Set up Docker | ||
- name: Upload charm to Charmhub | ||
env: | ||
CHARMHUB_TOKEN: ${{ secrets.CHARMHUB_TOKEN }} | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
run: | | ||
sudo snap install docker | ||
sudo addgroup --system docker | ||
sudo adduser $USER docker | ||
newgrp docker <<< "sudo snap disable docker" | ||
newgrp docker <<< "sudo snap enable docker" | ||
- name: Upload charm to charmhub | ||
uses: canonical/charming-actions/[email protected] | ||
with: | ||
credentials: "${{ secrets.CHARMHUB_TOKEN }}" | ||
github-token: "${{ secrets.GITHUB_TOKEN }}" | ||
channel: "${{ steps.channel.outputs.name }}" | ||
built-charm-path: "${{ matrix.path }}" | ||
charm-path: "${{ inputs.charm-path }}" | ||
tag-prefix: "${{ inputs.release-tag-prefix }}" | ||
charmcraft-channel: "${{ inputs.charmcraft-channel }}" | ||
# We set destructive mode to false, otherwise runner's OS would have to match | ||
# charm's 'build-on' OS. | ||
destructive-mode: false | ||
pip install git+https://github.com/lucabello/noctua | ||
lucabello marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cd "${{ inputs.charm-path }}" | ||
release=$(noctua charm release --path="${{ matrix.path }}" --channel=${{ inputs.release-channel }} --json | tail -n1) | ||
revision=$(echo "$release" | jq .revision) | ||
# TODO: push git tag; is it necessary? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand this comment. |
||
gh release create "${{inputs.release-tag-prefix}}rev${revision}" --title="Revision ${revision}" --generate-notes |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,11 @@ on: | |
type: boolean | ||
default: false | ||
required: true | ||
|
||
track: | ||
type: choice | ||
description: Track on which to run the promotion train | ||
options: | ||
- latest | ||
jobs: | ||
promote: | ||
name: Promote Charm | ||
|
@@ -52,52 +56,7 @@ jobs: | |
echo "charm_path=$CHARM_PATH" >> $GITHUB_OUTPUT; | ||
fi | ||
|
||
- name: Check which tracks already have a release | ||
id: check-tracks | ||
env: | ||
CHARMCRAFT_AUTH: ${{ secrets.CHARMHUB_TOKEN }} | ||
run: | | ||
sudo snap install charmcraft --classic | ||
cd ${{ steps.read-charm-path.outputs.charm_path }} | ||
charm_name=$(yq .name metadata.yaml 2>/dev/null || yq .name charmcraft.yaml) | ||
status=$(charmcraft status "$charm_name" --format=json) | ||
to_stable=$(echo "$status" | jq -r '.[] | select(.track == "latest") | .mappings[].releases[] | select(.channel == "latest/stable") | .status' | head -n1) | ||
to_candidate=$(echo "$status" | jq -r '.[] | select(.track == "latest") | .mappings[].releases[] | select(.channel == "latest/candidate") | .status' | head -n1) | ||
to_beta=$(echo "$status" | jq -r '.[] | select(.track == "latest") | .mappings[].releases[] | select(.channel == "latest/beta") | .status' | head -n1) | ||
echo "to_stable=$to_stable" >> $GITHUB_OUTPUT | ||
echo "to_candidate=$to_candidate" >> $GITHUB_OUTPUT | ||
echo "to_beta=$to_beta" >> $GITHUB_OUTPUT | ||
|
||
- name: (dry run) Print promotions | ||
if: ${{ inputs.dry-run }} | ||
- name: Run the promotion train | ||
run: | | ||
echo "${{ matrix.charm-repo }} would promote the following channels:" | ||
if [[ "${{ steps.check-tracks.outputs.to_stable }}" == "open" ]]; then echo "- latest/candidate --> latest/stable"; fi | ||
if [[ "${{ steps.check-tracks.outputs.to_candidate }}" == "open" ]]; then echo "- latest/beta --> latest/candidate"; fi | ||
if [[ "${{ steps.check-tracks.outputs.to_beta }}" == "open" ]]; then echo "- latest/edge --> latest/beta"; fi | ||
|
||
- name: Promote charm - latest/candidate --> latest/stable | ||
if: ${{ !inputs.dry-run && steps.check-tracks.outputs.to_stable == 'open' }} | ||
uses: canonical/charming-actions/[email protected] | ||
with: | ||
charm-path: ${{ steps.read-charm-path.outputs.charm_path }} | ||
credentials: ${{ secrets.CHARMHUB_TOKEN }} | ||
origin-channel: latest/candidate | ||
destination-channel: latest/stable | ||
|
||
- name: Promote charm - latest/beta --> latest/candidate | ||
if: ${{ !inputs.dry-run && steps.check-tracks.outputs.to_candidate == 'open' }} | ||
uses: canonical/charming-actions/[email protected] | ||
with: | ||
charm-path: ${{ steps.read-charm-path.outputs.charm_path }} | ||
credentials: ${{ secrets.CHARMHUB_TOKEN }} | ||
origin-channel: latest/beta | ||
destination-channel: latest/candidate | ||
- name: Promote charm - latest/edge --> latest/beta | ||
if: ${{ !inputs.dry-run && steps.check-tracks.outputs.to_beta == 'open' }} | ||
uses: canonical/charming-actions/[email protected] | ||
with: | ||
charm-path: ${{ steps.read-charm-path.outputs.charm_path }} | ||
credentials: ${{ secrets.CHARMHUB_TOKEN }} | ||
origin-channel: latest/edge | ||
destination-channel: latest/beta | ||
pip install git+https://github.com/lucabello/noctua | ||
noctua charm promote-train --track=${{ inputs.track }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ name: Promote Charm | |
on: | ||
workflow_call: | ||
inputs: | ||
charm-path: | ||
charm-name: | ||
description: "Path to the charm we want to promote. Defaults to the current working directory." | ||
type: string | ||
default: '.' | ||
|
@@ -27,25 +27,23 @@ jobs: | |
steps: | ||
- name: Checkout | ||
uses: actions/checkout@v4 | ||
- name: Setup Python | ||
uses: actions/setup-python@v4 | ||
- name: Set target channel | ||
env: | ||
PROMOTE_FROM: ${{ github.event.inputs.promotion }} | ||
run: | | ||
if [ "${PROMOTE_FROM}" == "edge -> beta" ]; then | ||
echo "promote-from=edge" >> ${GITHUB_ENV} | ||
echo "promote-to=beta" >> ${GITHUB_ENV} | ||
elif [ "${PROMOTE_FROM}" == "beta -> candidate" ]; then | ||
echo "promote-from=beta" >> ${GITHUB_ENV} | ||
echo "promote-to=candidate" >> ${GITHUB_ENV} | ||
elif [ "${PROMOTE_FROM}" == "candidate -> stable" ]; then | ||
echo "promote-from=candidate" >> ${GITHUB_ENV} | ||
echo "promote-to=stable" >> ${GITHUB_ENV} | ||
fi | ||
- name: Promote Charm | ||
uses: canonical/charming-actions/[email protected] | ||
with: | ||
charm-path: ${{ inputs.charm-path }} | ||
credentials: ${{ secrets.CHARMHUB_TOKEN }} | ||
destination-channel: ${{ github.event.inputs.track }}/${{ env.promote-to }} | ||
origin-channel: ${{ github.event.inputs.track }}/${{ env.promote-from }} | ||
charmcraft-channel: latest/stable | ||
env: | ||
CHARMHUB_TOKEN: ${{ secrets.CHARMHUB_TOKEN }} | ||
run: | | ||
pip install git+https://github.com/lucabello/noctua | ||
cd ${{ inputs.charm-path }} | ||
noctua charm promote --from=${{ github.event.inputs.track }}/${{ env.promote-from }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have to be in a charm dir to run |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,49 +69,13 @@ jobs: | |
id: update-releases | ||
if: steps.changed-files.outputs.all_changed_and_modified_files != '' | ||
run: | | ||
# Get the tags already in OCI Factory | ||
existing_tags="$(jq -r 'to_entries[] | .key' $GITHUB_WORKSPACE/oci-factory/oci/${{ inputs.rock-name }}/_releases.json | sed 's/-22.04//')" | ||
# Get the versions from the rocks that have been modified | ||
modified_tags="$(echo ${{ steps.changed-files.outputs.all_changed_and_modified_files }} | tr ' ' '\n' | sed s@/rockcraft.yaml@@)" | ||
# Merge the two to make a list of all the versions that will be in OCI Factory with | ||
# the PR that is being opened in this workflow | ||
all_tags="$existing_tags\n$modified_tags" | ||
today="$(date)" | ||
echo "now_epoch=$(date -d now +%s)" >> $GITHUB_OUTPUT # to create a unique branch name on the fork | ||
end_of_life="$(date -d "$today+3 months" +%Y-%m-%d)" | ||
yq -i ".upload = []" $GITHUB_WORKSPACE/oci-factory/oci/${{ inputs.rock-name }}/image.yaml | ||
for file in ${{ steps.changed-files.outputs.all_changed_and_modified_files }}; do | ||
# For each rock version, build the `upload:` element for image.yaml as a json | ||
# Example: {"source": "canonical/prometheus-rock", "commit": "...", ...} | ||
patch_tag=""; minor_tag=""; major_tag="" | ||
tag_json_format='"%s-22.04": {"end-of-life": "%sT00:00:00Z", "risks":["stable"]}' | ||
# Parse the rock version from the rockcraft.yaml | ||
rock_version=$(yq -r '.version' $GITHUB_WORKSPACE/rock/$file) | ||
# Always tag with patch | ||
patch_tag=$(printf "$tag_json_format" "$rock_version" "$end_of_life") | ||
# If rock_version is the latest tag among the ones with equal major.minor, apply major.minor | ||
rock_major_minor=$(echo $rock_version | sed -E "s/([0-9]+\.[0-9]+).*/\1/") | ||
same_major_minor=$(printf "%s\n%s" "$all_tags" "$rock_version" | grep "$rock_major_minor") | ||
if [[ $(echo "$same_major_minor" | sort -V | tail -n1) == "$rock_version" ]]; then | ||
minor_tag=$(printf ",$tag_json_format" "$rock_major_minor" "$end_of_life") | ||
fi | ||
# If rock_version is the latest among the ones with equal major, apply major | ||
rock_major=$(echo $rock_version | sed -E "s/([0-9]+).*/\1/") | ||
same_major=$(printf "%s\n%s" "$all_tags" "$rock_version" | grep "$rock_major") | ||
if [[ $(echo "$same_major" | sort -V | tail -n1) == "$rock_version" ]]; then | ||
major_tag=$(printf ",$tag_json_format" "$rock_major" "$end_of_life") | ||
fi | ||
# Build the final JSON object to update image.yaml | ||
rock_tags=$(printf '{%s%s%s}' "$patch_tag" "$minor_tag" "$major_tag") | ||
upload_item_format='{"source":"%s","commit":"%s","directory":"%s","release":%s}' | ||
upload_item=$(printf "$upload_item_format" \ | ||
"canonical/${{ inputs.rock-name }}-rock" \ | ||
"${{ steps.commit-sha.outputs.commit_sha }}" \ | ||
"$rock_version" \ | ||
"$rock_tags" \ | ||
) | ||
yq -i ".upload += $upload_item" $GITHUB_WORKSPACE/oci-factory/oci/${{ inputs.rock-name }}/image.yaml | ||
done | ||
pip install git+https://github.com/lucabello/noctua | ||
versions="$(echo ${{ steps.changed-files.outputs.all_changed_and_modified_files }} | tr ' ' '\n' | grep rockcraft.yaml | sed 's@/rockcraft.yaml@@g')" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this something noctua should do? |
||
noctua rock manifest "${{ github.repository }}" \ | ||
--commit="${{ steps.commit-sha.outputs.commit_sha }}" \ | ||
--base=22.04 \ | ||
$(echo $versions | sed -E 's/(\S+)/--version \1/g' | tr '\n' ' ') \ # build the --version flags | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this something noctua should do? |
||
> $GITHUB_WORKSPACE/oci-factory/oci/${{ inputs.rock-name }}/image.yaml | ||
|
||
- name: Commit to the fork | ||
id: fork-commit | ||
|
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.
Prior to merging this PR, this is a really good time to start doing releases in this repo. That way if a single repo needs to revert because of a bug, they can point to the previous version of the workflows
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'm a bit hesitant to do that until we have automatic machinery to bump the versions. At the moment, having all the repos update automatically is a feature more than a bug. We could configure renovate in all of our repos to look at this version specifically, but maybe I'd rather do that at a point where we're
pip
-installing charm libraries.You're right saying we should have this, but I think we need to have Renovate set up first.
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.
yeah agreed that, without automation, this suggestion adds a lot of chore.
Because this repo has such a large blast radius (a break in this repo can block all work (PRs, releases) in the other repos), I worry about big changes without versioning and testing. Its not a hard blocker, just a worry
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.
We're not at 100%, but what we get from this PR is actually way more tested than what we had before.
We didn't test charming actions; we had (and still have) some ARM releases broken for 3 months without noticing, because there was no testing involved at all. Here
noctua
is unit-tested, we have way more guarantees than before.Also, if a break in this repo broke the PR CI of something, we'd just revert this PR. It's not like as we merge, every charm will run the release CI immediately. If there's an issue, it will pop up in one charm, and we'll fix it/revert this PR and work on the fix.
I think your concern is right, but imho this is a step forward compared to what we had before.
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 agree this is better tested than the previous version when the previous version was written, but today we have plenty of production use showing the current version works. A refactor still feels risky. That this version has gone through better testing that the last one is good, but doesn't convince me that we shouldn't version our releases