feat: use noctua in our CI #207
Conversation
| 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.
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.
Good point, I will add this :)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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.
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.
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.
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
There was a problem hiding this comment.
Lots of really good stuff here and it does simplify the workflows a lot. My main concern is just that it is a lot of change to do without automated tests.
This sort of change is also really hard to review in detail (I can see directionally where you are going and like it, but its hard for me to ensure syntactically its actually correct), in part because of so much changing and in part because its a CI workflow. This PR should link CI runs that use this branch now so we can see it actually works.
I also suspect the workflows have not yet been tested? I tried to reproduce a few commands locally and hit errors (flags/options missing, etc). I also see no --no-dry-run flags, which I think means none of the noctua calls are doing anything
Before this lands, we should also have a real review of the lucabot package
There was a problem hiding this comment.
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.
Can you give an example of what metadata you're referring to?
| # 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.
Have you considered packaging this not as run commands in a workflow, but as a python action? Then you get the benefit of easy python coding for the logic, but also the benefit of calling github actions (automated version bumping via renovate, simpler workflow files here that don't show implementation details, etc)
There was a problem hiding this comment.
oh, and testing. makes testing possible without needing the whole system
There was a problem hiding this comment.
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 charm-release.yaml to look at; if you're opening this, you arguably want to know what's happening.
I'd also argue that looking at a few lines of bash is a lot simpler than having another level of nesting (release.yaml in charms calls charm-release.yaml, which calls _charm_release.yaml, which should call yet another upload action).
There was a problem hiding this comment.
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 noctua code is unit-tested though. Once we have the automation in place, we can get automated version bumping of that tool instead. Renovate is useful only if we have a few requirements:
- we have a way to test the new action in the PR, which doesn't make much sense to me; we don't want to test when updating the CI in Prometheus/Grafana/all of our charms, we want to test before merging in
canonical/observability; - the automatic change saves us PR work; here, having this job as an action only produces one PR (to this repo, assuming the actions live somewhere else; otherwise you'd need to remember to bump manually); the CI on the actual charms would pick up a new tag on the whole
releaseworkflow, which would happen regardless
There was a problem hiding this comment.
Re using composite actions, agreed it is subjective. I like the benefits of:
- calling an action with a name that state's its purpose
- being able to test the entire step, not just the noctua portion
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.
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)
| 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? | ||
| gh release create "${{inputs.release-tag-prefix}}rev${revision}" --title="Revision ${revision}" --generate-notes |
There was a problem hiding this comment.
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 gh calls should be included so they aren't showing here (same question for other workflows too)
If noctua stayed as-is but the gh ... logic was hidden in a composite action like mentioned above, I'd feel less confused
There was a problem hiding this comment.
A problem with adding that to noctua is that the tool starts doing too much; one of the complaint I had about charming-actions is that you had no idea a GitHub release was happening.
I'm trying to follow the "path of least surprises", trying to stay closer to charmcraft than to charming-actions
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.
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
|
@ca-scribner I was writing a long comment, but then I realized it was too long, and so I just put it as PR description |
| @@ -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.
are we sure about this default channel?
There was a problem hiding this comment.
We are; we currently release stuff to latest/edge when we merge to main
|
After a meeting with @ca-scribner, I'm putting this in draft again, it'll be back with updates! |
| 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.
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?
| charm-path: "${{ inputs.charm-path }}" | ||
| charmcraft-channel: "${{ inputs.charmcraft-channel }}" | ||
| run: | | ||
| pip install git+https://github.com/lucabello/noctua | ||
| outdated_libs=$(noctua charm libraries check --minor | jq 'length') |
There was a problem hiding this comment.
A somewhat big ask, that we could leave for a future task:
Can you add a comment linking to the schema or an example output of each noctua command?
Alternatively could add an arg, e.g.
noctua charm libraries check --minor --sum-outdated
| 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) |
There was a problem hiding this comment.
Should we use something less fragile than tail -n1?
| 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.
I don't understand this comment.
I know we do push tags and I think it is necessary because currently it's our only way of linking revisions to commit.
| 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.
Do we have to be in a charm dir to run promote? Why?
| 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.
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.
Is this something noctua should do?
Some commands don't have the
--no-dry-runor flags that don't exist because I was adding things innoctuawhich I didn't push on Friday, so I resolved those comments.Composite actions vs.
run:with BashComposite actions have almost no benefits compared to this set of Bash commands.
canonical/observabilityrepo, not to a tag of the action; the situations "I update the Python action and version bump" vs "I update the Bash lines" both require a new tag incanonical/observability, and thus are functionally equivalentrun:, because it doesn't hide what is happening behind the scenes; this workflow is two layers of abstraction away from the caller anyway (some-charm/release.yaml->observability/charm-release.yaml->observability/_charm-release.yaml). Having yet another one (->observability/actions/charm-release) seems pointless to me.run:you can run locally, and if you don't know what they do you can just run--help.The one benefit you get from composite actions in this case is being able to unit-test the entire action, instead of just the most important fragment (i.e.
noctua). I think I'd rather test changes on a fake repository where we run the actual workflows instead.Another big point I have against composite actions is that no matter what, we need some Bash to glue things together. Obviously, you can just take all the current
run:steps and turn each into an action, but then I can just add those commands intonoctua. If having a one-line invocation is so important, of course we can havenoctuarun agh releasecommand internally, but I don't really see the point.I think keeping the codebase leaner and more modular makes things easier to test and produces less bugs overall.
How do we test this?
I completely understand, in general, the need to test changes before we release them. However, we've done changes to our CI with 0% testing so far. We likely need a "fake repository" infrastructure to see our workflows run before merging them. We likely need automation (e.g. Renovate) to version our CI, have some way to test it on the specific repo, then merge it. However, we don't have any of those things.
What this PR does is to go from 0% testing to 50% testing:
noctuais unit-tested on its own, the workflows are a lot simpler, and if you need to test a workflow you can run those commands manually! You want to test therun:section inupload-charm? Pick a charm likeflog, and run the commands manually in your terminal, dry-run or not. That's something we've never been able to do until now.I understand this doesn't guarantee things will work out-of-the-box, and there's still ways to improve from this 50%, but I believe it's still a major improvement compared to what we had before.