-
Notifications
You must be signed in to change notification settings - Fork 2
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: python test, lint and policy check workflows #18
feat: python test, lint and policy check workflows #18
Conversation
b5dacfd
to
3f143ef
Compare
444d04f
to
a075374
Compare
a075374
to
e982fea
Compare
550e805
to
b594ddd
Compare
799da0d
to
ec16d07
Compare
90d09d6
to
ed9968f
Compare
ed9968f
to
a0d610e
Compare
Re-requesting review from @tigarmo and @sergiusens given the changes made. |
required: false | ||
type: string | ||
default: '["ubuntu-22.04", "ubuntu-24.04", "windows-latest", "macos-latest"]' | ||
default: '["jammy", "noble"]' |
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.
don't we need to further restrict these to amd64
?
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 can and we needn't. I added the architectures in the self-test in order to check that we could add lists of tags, but I figured here it would be a good default to simply run the tests on the first available runner regardless of architecture.
.github/workflows/test-python.yaml
Outdated
uses: astral-sh/setup-uv@v3 | ||
with: | ||
enable-cache: true | ||
cache-suffix: ${{ toJSON(matrix.platform) }} |
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.
Based on the errors in https://github.com/canonical/starflow/actions/runs/11824136927 I think the conversion back to a JSON is giving an invalid cache key.
We could try with:
cache-suffix: ${{ toJSON(matrix.platform) }} | |
cache-suffix: ${{ matrix.platform }} |
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.
The path doesn't exist because we're not actually using uv
in those tests. This needs to be toJSON
so it's a string - otherwise it fails when you have an array of tags (e.g. your platform is our self-hosted [noble, amd64]
).
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.
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.
Ahh, I see. Thank you! Fixed :-)
Co-authored-by: Upils <[email protected]>
Co-authored-by: Upils <[email protected]>
Co-authored-by: Upils <[email protected]>
88d1860
to
91206f9
Compare
91206f9
to
e8b4a5c
Compare
8edf6fa
to
a5112a1
Compare
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.
LGTM. But I still have a small concern about the cache keys names, see #18 (comment)
b57d2a0
to
698fa3b
Compare
This adds three workflows:
These are detailed in
README.md
Sample usage:
Intentional failures: