Conversation
|
|
||
| This guide demonstrates how to automatically run your charm's tests against any PR into the main branch of your GitHub repository. | ||
|
|
||
| You might also want to automatically publish your charm on Charmhub or publish charm libraries on PyPI. [charming-actions](https://github.com/canonical/charming-actions) has some useful GitHub actions for publishing on Charmhub. For guidance about publishing on PyPI, see {external+charmlibs:ref}`How to distribute charm libraries <python-package-distribution-pypi>`. |
There was a problem hiding this comment.
I wonder if we should keep endorsing charming-actions.
There was a problem hiding this comment.
I think we should not. But I'm not sure whether we should change that in this PR or later on. I feel like it's a separate PR where we figure out for each feature that charming-actions offers what the Charm Tech approved alternative is, and then update our docs (and potentially your spec) accordingly.
There's also the charmlibs bit. There's still value in the various actions for managing Charmhub charmlibs, but I'm not sure we want to promote those in our docs, unless it's in a migration guide or legacy page.
Maybe this should be a roadmap item for 26.10?
It seems to me like it would be simpler to keep the scope of this PR smaller, and focus on consolidating the content more than updating it, except in very straightforward cases.
But also:
| You might also want to automatically publish your charm on Charmhub or publish charm libraries on PyPI. [charming-actions](https://github.com/canonical/charming-actions) has some useful GitHub actions for publishing on Charmhub. For guidance about publishing on PyPI, see {external+charmlibs:ref}`How to distribute charm libraries <python-package-distribution-pypi>`. | |
| You might also want to automatically publish your charm on Charmhub or publish charm libraries on PyPI. [charming-actions](https://github.com/canonical/charming-actions) has some useful GitHub actions for publishing on Charmhub. For guidance about publishing libraries on PyPI, see {external+charmlibs:ref}`How to distribute charm libraries <python-package-distribution-pypi>`. |
|
|
||
| Integration tests require a Juju controller and a cloud in which to deploy your charm. We recommend that you use [Concierge](https://github.com/canonical/concierge) to prepare the CI environment. | ||
|
|
||
| If your charm is a Kubernetes charm, create a file called `.github/workflows/integration-tests.yaml`: |
There was a problem hiding this comment.
How about we recommend putting everything in a ci.yaml file for now? The integration test job is simple enough to live in the same file as the rest.
There was a problem hiding this comment.
If we do that, then we should leave the workflow_dispatch in place.
| workflow_dispatch: | ||
|
|
||
| jobs: | ||
| integration: |
There was a problem hiding this comment.
If we put this in the same file, how about adding needs: [lint, unit]?
There was a problem hiding this comment.
I don't think integration tests need linting and unit tests to pass in order to run. In CI, I would rather see everything that's going wrong rather than seeing some formatting failure and having to fix that before I can see the integration test failures, rather than seeing all of them at once.
There was a problem hiding this comment.
Well maybe just needs: [unit] then if you want to reduce the chance that we're blocking an integration test run that would otherwise succeed or fail in an interesting way?
I personally find it's well worth having to quickly fix bugs that would be caught by lint/unit before the integration tests will run, if only to avoid getting emails 15 minutes later because bad code from an old commit was being packed and deployed in the background and now finally failed due to a missing import or similar that was fixed when unit tests run.
I guess it is a matter of taste though, and arguably a skill issue if you push code that would fail lint/unit, so if we want to stick with Charm Tech's recommendation being to start your integration tests ASAP, it's not a blocker for me.
| run: tox -e integration -- --model testing | ||
| - name: Dump logs | ||
| uses: canonical/charm-logdump-action@main | ||
| if: failure() | ||
| with: | ||
| app: your-app | ||
| model: testing |
There was a problem hiding this comment.
Would we need to make sure the charm has pytest-jubilant in its integration testing deps?
There was a problem hiding this comment.
Yes. I think that's OK, as charm-logdump-action makes some pretty strong assumptions too (microk8s only, models aren't torn down by integration tests). We should document that we're assuming pytest-jubilant integration tests, of course.
There was a problem hiding this comment.
The comment link doesn't seem to be working. Trying this link instead #2406 (comment)
tonyandrewmeyer
left a comment
There was a problem hiding this comment.
Thanks! This looks good to me. I think it's best to keep it more-or-less like it is now, and then make adjustments to what we recommend, particularly with regards to external actions/workflows, in subsequent PRs. Partly so that this one stays focused, but partly because I think we haven't answered for ourselves exactly what we're recommending yet.
|
|
||
| This guide demonstrates how to automatically run your charm's tests against any PR into the main branch of your GitHub repository. | ||
|
|
||
| You might also want to automatically publish your charm on Charmhub or publish charm libraries on PyPI. [charming-actions](https://github.com/canonical/charming-actions) has some useful GitHub actions for publishing on Charmhub. For guidance about publishing on PyPI, see {external+charmlibs:ref}`How to distribute charm libraries <python-package-distribution-pypi>`. |
There was a problem hiding this comment.
I think we should not. But I'm not sure whether we should change that in this PR or later on. I feel like it's a separate PR where we figure out for each feature that charming-actions offers what the Charm Tech approved alternative is, and then update our docs (and potentially your spec) accordingly.
There's also the charmlibs bit. There's still value in the various actions for managing Charmhub charmlibs, but I'm not sure we want to promote those in our docs, unless it's in a migration guide or legacy page.
Maybe this should be a roadmap item for 26.10?
It seems to me like it would be simpler to keep the scope of this PR smaller, and focus on consolidating the content more than updating it, except in very straightforward cases.
But also:
| You might also want to automatically publish your charm on Charmhub or publish charm libraries on PyPI. [charming-actions](https://github.com/canonical/charming-actions) has some useful GitHub actions for publishing on Charmhub. For guidance about publishing on PyPI, see {external+charmlibs:ref}`How to distribute charm libraries <python-package-distribution-pypi>`. | |
| You might also want to automatically publish your charm on Charmhub or publish charm libraries on PyPI. [charming-actions](https://github.com/canonical/charming-actions) has some useful GitHub actions for publishing on Charmhub. For guidance about publishing libraries on PyPI, see {external+charmlibs:ref}`How to distribute charm libraries <python-package-distribution-pypi>`. |
| - main | ||
| pull_request: | ||
| workflow_call: | ||
| workflow_dispatch: |
There was a problem hiding this comment.
Is this useful for linting and unit tests? I feel like if you want to run those manually, you ought to be able to do that locally.
| (set-up-ci-linting-unit)= | ||
| ## Create a workflow for linting and unit tests | ||
|
|
||
| Create a file called `.github/workflows/tests.yaml`: |
There was a problem hiding this comment.
Let's Zizmor this file and fix the issues it'll report (like not storing git credentials, default permissions, etc). People will copy it exactly, so we should make it as safe as possible.
|
|
||
| Integration tests require a Juju controller and a cloud in which to deploy your charm. We recommend that you use [Concierge](https://github.com/canonical/concierge) to prepare the CI environment. | ||
|
|
||
| If your charm is a Kubernetes charm, create a file called `.github/workflows/integration-tests.yaml`: |
There was a problem hiding this comment.
If we do that, then we should leave the workflow_dispatch in place.
| workflow_dispatch: | ||
|
|
||
| jobs: | ||
| integration: |
There was a problem hiding this comment.
I don't think integration tests need linting and unit tests to pass in order to run. In CI, I would rather see everything that's going wrong rather than seeing some formatting failure and having to fix that before I can see the integration test failures, rather than seeing all of them at once.
| # Set a predictable model name so it can be consumed by charm-logdump-action. | ||
| run: tox -e integration -- --model testing | ||
| - name: Dump logs | ||
| uses: canonical/charm-logdump-action@main |
There was a problem hiding this comment.
I wonder about recommending this, too. It's a lot more than the Juju logs, but is it something that should be used by all (K8s) charms? If so, should Charm Tech be providing something for this ourselves?
This PR consolidates our various CI examples and puts them into a how-to guide so they are much easier to find.
Preview of the new guide
I tried to keep the guidance minimal and close to what we currently have. We can build on the guidance in follow-up PRs. But if the examples themselves need tweaking/improving, let's do that in this PR.
Merge this after #2406