Skip to content

Conversation

@hmeir
Copy link
Contributor

@hmeir hmeir commented Jan 14, 2024

Short description:

Datasource can have two types of bootsource: pvc and snapshot.
pvc property already exists, adding snapshot.

What this PR does / why we need it:

Adding snapshot property
refactor the "pvc" property so we access the boot source according to the type requested
if the pvc/snapshot doesn't exists, return None

Signed-off-by: Harel Meir <hmeir@redhat.com>
@redhat-qe-bot
Copy link
Contributor

Report bugs in Issues

The following are automatically added:

  • Add reviewers from OWNER file (in the root of the repository) under reviewers section.
  • Set PR size label.
  • New issue is created for the PR. (Closed when PR is merged/closed)
  • Run pre-commit if .pre-commit-config.yaml exists in the repo.

Available user actions:

  • To mark PR as WIP comment /wip to the PR, To remove it from the PR comment /wip cancel to the PR.
  • To block merging of PR comment /hold, To un-block merging of PR comment /hold cancel.
  • To mark PR as verified comment /verified to the PR, to un-verify comment /verified cancel to the PR.
    verified label removed on each new commit push.
  • To cherry pick a merged PR comment /cherry-pick <target branch to cherry-pick to> in the PR.
    • Multiple target branches can be cherry-picked, separated by spaces. (/cherry-pick branch1 branch2)
    • Cherry-pick will be started when PR is merged
  • To build and push container image command /build-and-push-container in the PR (tag will be the PR number).
  • To add a label by comment use /<label name>, to remove, use /<label name> cancel
Supported /retest check runs
  • /retest tox: Retest tox
  • /retest sonarqube: Retest sonarqube
  • /retest python-module-install: Retest python-module-install
Supported labels
  • hold
  • verified
  • wip
  • lgtm

Comment on lines +62 to +68
@property
def pvc(self):
return self._get_boot_source(boot_source_type="pvc")

@property
def snapshot(self):
return self._get_boot_source(boot_source_type="snapshot")
Copy link
Collaborator

Choose a reason for hiding this comment

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

boot_source should be the property that returns either a PVC or snapshot

Copy link
Collaborator

Choose a reason for hiding this comment

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

To clarify - add boot_source and add deprecation on pvc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify:

You are suggesting that the only property should be boot source,
And that it should return PersisentVolumeClaim or VolumeSnapshot dynamically based on the the instance boot source type?

Copy link
Collaborator

Choose a reason for hiding this comment

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

right, and add deprecation warning on pvc that it will be deprecated in 4.16.

Copy link
Contributor Author

@hmeir hmeir Jan 22, 2024

Choose a reason for hiding this comment

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

Hi

I have this PR in downstream.
And I want to apply this DataSource change on it as well, because its accessing its snapshot/pvc references.
Its currently on main, but need to be backported to 4.15 & 4.14.

So If I change to boot source instead of pvc, the implementation will be different in 4.14,4.15.
So should I implement the downstream PR with boot_source and in 4.14&4.15 add the snapshot property like I did here?

@hmeir hmeir closed this Jan 22, 2024
@hmeir hmeir deleted the datasource-snapshot-prop branch January 22, 2024 13:32
@hmeir hmeir restored the datasource-snapshot-prop branch January 22, 2024 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants