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

Fix test regression on LXD 4.0 #594

Merged
merged 4 commits into from
Jul 8, 2024

Conversation

hamistao
Copy link
Contributor

@hamistao hamistao commented Jul 5, 2024

Fixes #590.
Unfortunately, the best possible solution for create is picking the latest snapshot name when getting the object after creation.
This is prone to error if multiple snapshots of the same volume are being taken concurrently. But since this isn't a realistic use case, these changes should be acceptable in favor of not hurting UX by returning an empty object.

@hamistao hamistao requested a review from simondeziel July 5, 2024 05:58
@hamistao hamistao force-pushed the fix_snapshot_crete_regression branch 2 times, most recently from 062f30c to 76bf24b Compare July 5, 2024 06:25
@hamistao hamistao force-pushed the fix_snapshot_crete_regression branch from 76bf24b to 5694ecb Compare July 5, 2024 13:50
Copy link
Member

@simondeziel simondeziel 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 rebasing to get testing on 4.0 :)

pylxd/models/storage_pool.py Outdated Show resolved Hide resolved
@hamistao hamistao force-pushed the fix_snapshot_crete_regression branch from 5694ecb to f6a01d2 Compare July 5, 2024 19:28
@hamistao hamistao changed the title pylxd: Use latest snapshot name if necessary Fix test regression on LXD 4.0 Jul 5, 2024
@hamistao hamistao force-pushed the fix_snapshot_crete_regression branch 8 times, most recently from f014738 to b5ebdc5 Compare July 5, 2024 22:21
Copy link
Member

@simondeziel simondeziel left a comment

Choose a reason for hiding this comment

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

Please better explain the rational for the content_type and create_at fiddling as I don't fully understand why they need tweaking :/

pylxd/models/storage_pool.py Outdated Show resolved Hide resolved
@hamistao hamistao force-pushed the fix_snapshot_crete_regression branch from e6d2aac to 7c3ff77 Compare July 5, 2024 23:27
@hamistao hamistao marked this pull request as draft July 8, 2024 15:37
@hamistao hamistao force-pushed the fix_snapshot_crete_regression branch 6 times, most recently from 8508894 to 2bd52ef Compare July 8, 2024 18:49
@hamistao
Copy link
Contributor Author

hamistao commented Jul 8, 2024

@simondeziel This is ready for a review. The changes should be better explained on the commit messages, if anything is still confusing let me know.

@hamistao hamistao marked this pull request as ready for review July 8, 2024 18:54
Copy link
Member

@simondeziel simondeziel left a comment

Choose a reason for hiding this comment

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

The content_type handling is the last remaining concern/question I have.

Older LXD versions' APIs don't include:
The `created_at` field,
the `expires_at` field on /1.0/storage-pools/<pool>/volumes/custom/<volume_name>/snapshots,
The `content_type` on /1.0/storage-pools/<pool>/volumes/custom/<volume_name>/snapshots/<snapshot_name>.

Signed-off-by: hamistao <[email protected]>
@hamistao hamistao force-pushed the fix_snapshot_crete_regression branch from 2bd52ef to 9137f09 Compare July 8, 2024 19:52
@hamistao
Copy link
Contributor Author

hamistao commented Jul 8, 2024

@simondeziel This should be ready to merge

@simondeziel simondeziel merged commit de04a29 into canonical:main Jul 8, 2024
14 checks passed
@hamistao hamistao deleted the fix_snapshot_crete_regression branch July 18, 2024 02:55
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.

Integration test regression on LXD 4.0
3 participants