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

Move VA1 to the new GlanceAPI definition #32

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Move VA1 to the new GlanceAPI definition #32

merged 1 commit into from
Nov 16, 2023

Conversation

fmount
Copy link
Contributor

@fmount fmount commented Nov 15, 2023

Glance moved to a new API definition via [1][2] and this patch aligns VA1 to the current changes.

Depends-On: openstack-k8s-operators/openstack-operator#514

[1] openstack-k8s-operators/glance-operator#329
[2] openstack-k8s-operators/openstack-operator#514

@fmount fmount requested review from abays and fultonj November 15, 2023 21:33
@fmount fmount changed the title Mova VA1 to the new GlanceAPI definition DNM - Move VA1 to the new GlanceAPI definition Nov 15, 2023
@fmount fmount marked this pull request as draft November 15, 2023 21:46
@fmount fmount changed the title DNM - Move VA1 to the new GlanceAPI definition Move VA1 to the new GlanceAPI definition Nov 15, 2023
@fmount
Move VA1 to the new GlanceAPI definition
ba4a2e6 
Glance moved to a new API definition via [1][2] and this patch aligns
VA1 to the current changes.

[1] openstack-k8s-operators/glance-operator#329
[2] openstack-k8s-operators/openstack-operator#514

Signed-off-by: Francesco Pantano <fpantano@redhat.com>
fultonj
fultonj reviewed Nov 15, 2023
View reviewed changes
Copy link
Contributor

@fultonj fultonj left a comment

Choose a reason for hiding this comment

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

I'm +2 on this after the depends-on merges

abays
abays approved these changes Nov 16, 2023
View reviewed changes
Copy link
Contributor

@abays abays left a comment

Choose a reason for hiding this comment

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

openstack-k8s-operators/openstack-operator#514 has merged

/lgtm

@fmount fmount marked this pull request as ready for review November 16, 2023 11:04
@fmount
Copy link
Contributor Author

fmount commented Nov 16, 2023

Thanks, I guess we are ready to go here.
However, before merging, I'm wondering if the VA1 is used in combination w/ DP2 content. In that case, shouldn't we need to branch here before merging this change? In theory just a DP2 tag should work as well.
@abays wdyt?

@abays
Copy link
Contributor

abays commented Nov 16, 2023

Thanks, I guess we are ready to go here. However, before merging, I'm wondering if the VA1 is used in combination w/ DP2 content. In that case, shouldn't we need to branch here before merging this change? In theory just a DP2 tag should work as well. @abays wdyt?

We could do that, but VA1 has never been tested against DP2 even as-is prior to this PR. My worry would be that a DP2 branch would imply that it has indeed been validated against that.

I think going forward we will need to make specific, focused efforts to test against both upstream and downstream releases and pin branches/tags in the VAs to those.

@fmount fmount merged commit c4eefc0 into openstack-k8s-operators:main Nov 16, 2023
@fmount fmount deleted the new_glance_api branch November 16, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fultonj fultonj fultonj left review comments

@abays abays abays approved these changes

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
@fmount @abays @fultonj