refactor and add some unit tests

[simskij]

This PR is a quite substantial refactor built on top of @DomFleischmann's work with adding tag support to the action. In addition to just extracting and segmenting the code, it also adds linting, a bunch of unit tests, and formatting using prettier.

[jnsgruk]

Overall looking way better - nice work!

[jnsgruk]

This does make the assumption of access to the Docker socket through group membership or similar - is it worth catching the case where that doesn't happen? I kinda wish we could fall back to something lighter like skopeo in these cases

[simskij]

Makes sense! If the charmhub registry supports head requests that would be even better and not require us to shuffle all that data around just to immediately trash it afterwards. I will need to have a look however.

[simskij]

Spit into issue #22

[jnsgruk]

Probably worth a comment here to show what you're parsing for when it inevitably changes!

@facundobatista having the option for Charmcraft to have a more global JSON (or similar) output mode would be nice

[simskij]

Yeah, an --out json would definitely make life easier, and less brittle 😅

[jnsgruk]

Hmm. The reliance on upstream-source needs some documentation given that it isn't actually in the Metadata spec :)

[simskij]

Agreed. If upstream-source is not present, we have no other means of getting the image name IIRC, even without pinning, which is a bit of a bummer. Being able to set a default value for the oci-image would be a welcome addition in my opinion.

[jnsgruk]

Do we always want to glob for all Charm files?

[simskij]

Only one charm will be built when the action is run. This is just a clever hack to get around having to construct the full filename ourselves. For multiple charms in the same CI, one will have to utilize a build matrix.

[ca-scribner]

If you're refactoring this, I'd like to suggest splitting this action into two:

1. charmhub-upload-action: an action that takes charm dir and ch_channel and uploads the charm to the explicitly specified destination
2. branch-to-channel (or some better name): an action that handles the logic of "given that I'm a push to gh:main, I should be destined for ch:latest/edge", etc

I think this would make things more intuitive (who reads charmhub-upload-action and expects conditionally publishing to certain places in ch?), testable/easier to maintain, as well as more transferrable to other teams who don't share our opinions on when/where things should be published.

Separating these would let everyone in canonical inherit a bulletproof charmhub-upload-action that does simply that: upload a charm to somewhere you specify. Other teams could then decide if they want our branch-to-channel logic or they could develop their own (maybe they want their main to be for developing without publishing, and instead map gh:latest to ch:latest for publishing).

For our use, we can use a composite action which lets us define an action that uses other actions as it's steps. So a composite charmhub-upload-and-publish-action (or please some better name :) ) could invoke branch-to-channel and charmhub-upload-action together and work like Ken's original action (automatically pick the target channel + upload the charm)

Splitting them up also solves publishing of bundle files. For publishing bundle files the logic of how to map github branchs to charmhub tracks is different, but we can just use the upload-only action for those.

It is a bit more work, but I think separating the functions like this will save time in the end. With the composite action we can treat these as reusable and composable functions

[simskij]

... (wall of text from @ca-scribner 😅)

That's a really good suggestion! If we decide to do this, we should probably move all charmcraft-related actions into a monorepo, similar to the codeql action. That would also include merging the lib-checker action here (which would make sense). The end result would look something like this:

    - name: Validate Libs
      uses: canonical/charm-action/validate-libs@v1
        with:
          charm_path: ./observability-libs
          github_token: ${{ secrets.GITHUB_TOKEN }}
          charmhub_token: ${{ secrets.CHARMHUB_TOKEN }}
    - name: Select Channel
      uses: canonical/charm-action/select-channel@v1
      id: channel
    - name: Upload Charm
      uses: canonical/charm-action/upload-charm@v1
        with:
          channel: "${{ steps.channel.outputs.name }}"
          credentials: "${{ secrets.CHARMCRAFT_AUTH }}"
          charm-path: test-charm/
          upload-image: "true"

or, if you want to run the full composite:

    - uses: canonical/charm-action@v1
        with:
          charm_path: ./observability-libs
          github_token: ${{ secrets.GITHUB_TOKEN }}
          charmhub_token: ${{ secrets.CHARMHUB_TOKEN }}
          upload-image: "true"

The only downside I can see of this change (which I personally think is quite a huge one) is that we will no longer be able to be as opinionated about how the charm release automation workflow would work. The whole "giving people the ability to use them as they want", while obviously providing that benefit, also means people will pick you up on that offer. There is something to be said about "we do it this way and you will have to as well if you want to use this action".

WDYT?

cc @DomFleischmann and @jnsgruk

[jnsgruk]

WDYT?

cc @DomFleischmann and @jnsgruk

Sounds sensible to me :) We can still put what we think the right answer is into the operator-template, and we can document the preferred process in the style guide too.

[DomFleischmann]

WDYT?

cc @DomFleischmann and @jnsgruk

I really like the idea of having the actions in one monorepo and agree with @jnsgruk as long as we clearly state our best practices in the template and the docs we might still influence people in doing the right thing.

[simskij]

Merging this with Dom's PR to get tests to work. PRs to PR branches are not really something we've taken into account for this workflow.

[ca-scribner]

Yeah I like a repo of release related actions, maybe with some good how-to's as well giving nice examples of how to use them. Agreed that we lose some control on how people use them (eg: someone can take the upload-action and always link gh:main:ch:stable/latest for some reason), but I think that's ok. And agreed with @jnsgruk that we can still provide opinions via the operator-template repo as well as through whatever guides we provide. I could see opinionated composite actions as what we make really visible, but someone interested could reassemble the pieces how they see fit

My javascript is non-existent so my help probably isn't wanted there, but happy to help in speccing the tools/use cases or writing some of the composite actions