Skip to content

Conversation

@dwilding
Copy link
Contributor

@dwilding dwilding commented Jul 4, 2025

This PR uses the httpbin demo charm for the "Charmcraft Pack Test" check.

Since the charm doesn't use pyproject.toml (yet? see #1805), I've kept the step that modifies requirements.txt. And since we're getting the ops package from a git repo, I've added a step that adds git as a build package in charmcraft.yaml. The hello-kubecon charm already had git specified as a build package.

@dwilding dwilding marked this pull request as ready for review July 4, 2025 13:10
@dwilding dwilding requested a review from tonyandrewmeyer July 6, 2025 23:19
@dwilding
Copy link
Contributor Author

dwilding commented Jul 6, 2025

@tonyandrewmeyer would you mind having a look at this? (No urgency)

It follows on from discussion that Dima, James, and I were having in #1886.

Comment on lines +38 to +39
- name: Install yq
run: sudo snap install yq
Copy link
Contributor

Choose a reason for hiding this comment

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

yq can also be apt-installed, but I guess either is fine.

Copy link
Collaborator

@tonyandrewmeyer tonyandrewmeyer 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 switching to a local charm is definitely the right move, and httpbin-demo should be sufficient for now (I imagine that we'd want the example charms' integration tests to run at some point, and that might make this superfluous -- but maybe that will take too long, and we'd rather keep one specific one here).

We should also remove this file from .github/workflows/update-charm-tests.yaml since there's no revision to update any more.

@dwilding
Copy link
Contributor Author

dwilding commented Jul 7, 2025

We should also remove this file from .github/workflows/update-charm-tests.yaml since there's no revision to update any more.

Done, thanks!

echo "'parts' already exists in charmcraft.yaml"
exit 1
fi
yq --inplace '.parts.charm.build-packages = ["git"]' charmcraft.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: probably not great to whack old build packages in case one is added later, but then we'd notice very quickly that this job fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm only adding parts if it doesn't already exist (this check). So if we add a build package later, the job should fail immediately - if I've got my logic right!

@dwilding
Copy link
Contributor Author

dwilding commented Jul 8, 2025

I'm merging this now. We can revise further in a follow-up PR if needed. Thanks everyone!

@dwilding dwilding merged commit eacba3d into canonical:main Jul 8, 2025
43 checks passed
@dwilding dwilding deleted the ci-pack-httpbin branch July 8, 2025 06:54
- name: Install yq
run: sudo snap install yq

- name: Add 'git' as a build package
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Add 'git' as a build package
- name: Add 'git' as a build package

I guess our yaml linter doesn't care about comment content... or maybe we ain't got no yaml linter.

Copy link
Contributor Author

@dwilding dwilding Jul 16, 2025

Choose a reason for hiding this comment

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

@tonyandrewmeyer I seem to remember that you caught a bunch of unnecessary whitespace with pre-commit recently. Would that have been useful here? Would you mind explaining how you do that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Run uvx pre-commit install in the repo(s) that you want to use pre-commit. If they've already got a .pre-commit-config.yaml file, then any time you commit the configured checks will be run (against the files being committed - anything with modifications that isn't in the commit is temporarily stashed, and anything without changes is ignored).

For trailing whitespace (and a few of the other checks) it'll auto-fix for you as well, so you can commit (get failure), verify the fix (if needed), git add, and then commit again.

I don't actually know how this works if you commit outside a terminal (using an IDE for example) but I assume it shows you the output if it fails or something?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can also invoke pre-commit manually at any time to run the checks and fixes just once (e.g. uvx pre-commit).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah great, thank you! I confirmed that pre-commit will clean up this training space next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants