-
Notifications
You must be signed in to change notification settings - Fork 30
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!: add ability to retain snapshot after cleanup #407
base: main
Are you sure you want to change the base?
Conversation
pycloudlib/oci/cloud.py
Outdated
if not keep: | ||
self.created_images.append(image_data.id) | ||
else: | ||
self._log.info("Keeping image %s", image_data.display_name) |
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.
I think it's worth us persisting a list of self.kept_images
or self.retained_mages
. Then on teardown/cloud.clean()
we can make sure we emit a footer log or something conspicuous that says f"Skipping deletion of preserved image(s): {self.retained_images}"
. Otherwise there is little hope we will see this inline log during image creation during our busy integration tests.
Also we probably want to log both name and id so that we can reference that image in the future on CLI/SDK/cloud-dashboards.
self._log.info("Keeping image %s", image_data.display_name) | |
self._log.info("Keeping image %s[id:%s]", image_data.display_name, image_data.id) |
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.
Agreed! Thanks for the feedback @blackboxsw!
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.
Question for the maintainers: Do we want to make this an optional part of the standard pycloudlib API across clouds? I could easily add this to all other clouds if so.
I agree with your sentiment. I think it's a valuable feature to have generally across clouds. Let's make this something available in the general API of pycloudlib.Cloud.snapshot
.
2f08c98
to
009a7af
Compare
009a7af
to
8ed5695
Compare
1f5f311
to
8238077
Compare
5d4fad7
to
774bbbe
Compare
@blackboxsw finally ready for re-review! |
This allows for marking snapshots to be kept at time of creation instead of being cleaned up automatically. This allows for still using context managers for easy cleanup while retaining snapshots where desired. This functionality can also be used when not using context managers and instead the cloud.clean() method is manually called, like in cloud-init's integration tests. BREAKING CHANGE: signature of cloud.snapshot() public method has been changed and now all args except for the first (instance) must be passed as named args.
774bbbe
to
1b4a56c
Compare
This allows for marking snapshots to be kept at time of creation
instead of being cleaned up automatically. This allows for still using
context managers for easy cleanup while retaining snapshots where
desired, and also when not using context managers and the
cloud.clean() method is called.
Keeping this as a draft until the following is resolved.
Question for the maintainers: Do we want to make this an optional part of the standard pycloudlib API across clouds? I could easily add this to all other clouds if so.