Skip to content
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

Improve poetry adjustments in CI workflows #1272

Closed
tonyandrewmeyer opened this issue Jun 25, 2024 · 3 comments · Fixed by #1275
Closed

Improve poetry adjustments in CI workflows #1272

tonyandrewmeyer opened this issue Jun 25, 2024 · 3 comments · Fixed by #1275
Assignees
Labels
github_actions Pull requests that update GitHub Actions code low priority Non-urgent issue we may (or may not) consider later small item

Comments

@tonyandrewmeyer
Copy link
Contributor

Our workflows that test against charms currently patches in the appropriate version of ops:

      - name: Update 'ops' dependency in test charm to latest
        run: |
          if [ -e "requirements.txt" ]; then
            sed -i -e "/^ops[ ><=]/d" -e "/canonical\/operator/d" -e "/#egg=ops/d" requirements.txt
            echo -e "\ngit+$GITHUB_SERVER_URL/$GITHUB_REPOSITORY@$GITHUB_SHA#egg=ops" >> requirements.txt
          else
            sed -i -e "s/^ops[ ><=].*/ops = {path = \"myops\"}/" pyproject.toml
            poetry lock
          fi

With poetry, this has a side effect that the poetry lock will regenerate the entire lockfile, which could bump dependencies other than ops, which is not the intention for this workflow (but is probably a good idea for the charm itself).

@dimaqq suggested that if we do poetry remove ops and poetry add x that should only make the ops change, which would better match what we want. If that doesn't work, perhaps we can adjust the workflow in some other way to avoid this side effect.

@tonyandrewmeyer tonyandrewmeyer added small item low priority Non-urgent issue we may (or may not) consider later github_actions Pull requests that update GitHub Actions code labels Jun 25, 2024
@carlcsaposs-canonical
Copy link
Contributor

I'd suggest using poetry lock --no-update and I think you'll get the behavior you're looking for

(will update lock file for changes in pyproject.toml, but not other changes)

@carlcsaposs-canonical
Copy link
Contributor

you can also use poetry add to update pyproject.toml (no need to run poetry remove)

e.g. poetry add git+https://github.com/canonical/operator/ --lock

@tonyandrewmeyer
Copy link
Contributor Author

Thanks @carlcsaposs-canonical !

@tonyandrewmeyer tonyandrewmeyer self-assigned this Jun 25, 2024
amandahla pushed a commit to amandahla/operator that referenced this issue Jun 26, 2024
When running `poetry lock` in the CI workloads, only update the ops
dependency, not all dependencies. We want to know whether the proposed
changes to ops would break the charm's tests, not whether updating all
dependencies, including the proposed ops changes, would do so.

This is one of the suggestions in canonical#1272. Using `poetry add --lock` seems
slightly cleaner than the `sed` system we currently use, but I wasn't
able to easily figure out how to do that with a `ops = { path =
"ci/path/for/ops/branch" }` type specifier. It seems like this approach
is at least an improvement on the existing one, even if not perfect. (It
also will unblock other PRs, given that the tests fail because we're
re-locking in entirety).

Fixes canonical#1272.
tonyandrewmeyer added a commit to tonyandrewmeyer/operator that referenced this issue Jun 26, 2024
When running `poetry lock` in the CI workloads, only update the ops
dependency, not all dependencies. We want to know whether the proposed
changes to ops would break the charm's tests, not whether updating all
dependencies, including the proposed ops changes, would do so.

This is one of the suggestions in canonical#1272. Using `poetry add --lock` seems
slightly cleaner than the `sed` system we currently use, but I wasn't
able to easily figure out how to do that with a `ops = { path =
"ci/path/for/ops/branch" }` type specifier. It seems like this approach
is at least an improvement on the existing one, even if not perfect. (It
also will unblock other PRs, given that the tests fail because we're
re-locking in entirety).

Fixes canonical#1272.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code low priority Non-urgent issue we may (or may not) consider later small item
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants