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.

Sign up for GitHub

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

Jump to bottom

Update code for fixed issues in LXD #554

Merged
merged 6 commits into from
Jul 13, 2023
Merged

Update code for fixed issues in LXD #554

merged 6 commits into from
Jul 13, 2023

Conversation

simondeziel
Copy link
Member

@simondeziel simondeziel commented Jul 13, 2023 •
edited
Loading

This ended up fixing a small testing regression silently introduced by another PR.

pylxd/models/instance: only pass recursion if not 0

This fixes tox -e coverage. This was not caught when introduced by
PR #551 because we our CI job was still only triggering on the old
branch name. The CI job was fixed afterward by commit 094bab4.

The decision to omit the recursion param was taken because that's easier
than trying to fix the mocked HTTP responses.

@simondeziel
doc/instance: LXD issue 2201 was fixed long ago
f256d54 
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
@simondeziel
pylxd/models/instance: LXD issue 2201 was fixed long ago
a3cb0c7 
LXD now returns a clear error message if it runs out of space.

Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
@simondeziel
integration/test_storage: fix test_patch case and don't skip it
635e684 
canonical/lxd#4709 was fixed long ago
but the test case was wrong.

Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
@simondeziel
integration/test_storage: check initial description in test_put and t…
6169303 
…est_patch

Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
tomponline
tomponline previously approved these changes Jul 13, 2023
View reviewed changes
@simondeziel simondeziel marked this pull request as draft July 13, 2023 12:28
@simondeziel
pylxd/models/instance: blacken
0307879 
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
@simondeziel
pylxd/models/instance: only pass recursion if not 0
2a6c7fe 
This fixes `tox -e coverage`. This was not caught when introduced by
PR canonical#551 because we our CI job was still only triggering on the old
branch name. The CI job was fixed afterward by commit 094bab4.

The decision to omit the recursion param was taken because that's easier
than trying to fix the mocked HTTP responses.

Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
@codecov
Copy link

codecov bot commented Jul 13, 2023 •
edited
Loading

Codecov Report

Merging #554 (f8a9fa4) into main (e7d13e3) will decrease coverage by 0.19%.
The diff coverage is 50.00%.

❗ Current head f8a9fa4 differs from pull request most recent head 2a6c7fe. Consider uploading reports for the commit 2a6c7fe to get more accurate results

@@            Coverage Diff             @@
##             main     #554      +/-   ##
==========================================
- Coverage   97.43%   97.25%   -0.19%     
==========================================
  Files          56       56              
  Lines        4251     4262      +11     
==========================================
+ Hits         4142     4145       +3     
- Misses        109      117       +8
Impacted Files Coverage Δ
pylxd/models/instance.py 93.41% <50.00%> (-2.37%) ⬇️

@simondeziel simondeziel marked this pull request as ready for review July 13, 2023 12:54
@simondeziel simondeziel requested review from markylaing and removed request for markylaing July 13, 2023 14:32
@markylaing
Copy link
Contributor

@simondeziel I'm a little confused as to why CI didn't run for the last PR, requiring you to fix a few things in this one. I had to click a button to trigger the tests, but I think only the Commits tests ran.

markylaing
markylaing approved these changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@markylaing markylaing left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the bits I missed in the last PR 😉

@simondeziel
Copy link
Member Author

@markylaing the thing is that our CI job was only triggering on push/PRs toward master but we had renamed it to main already ;)

@tomponline tomponline merged commit 796f3f3 into canonical:main Jul 13, 2023
9 of 11 checks passed
@markylaing
Copy link
Contributor

@markylaing the thing is that our CI job was only triggering on push/PRs toward master but we had renamed it to main already ;)

Ohhh that makes sense 😆 never mind. I should have read the contributing guidelines more closely. My apologies.

@simondeziel simondeziel deleted the old-fixed-issues branch July 13, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markylaing markylaing markylaing approved these changes

@tomponline tomponline tomponline left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@simondeziel @markylaing @tomponline