Skip to content

containerinfra: add a few missing labels fields#2377

Merged
mandre merged 2 commits intogophercloud:masterfrom
MrFreezeex:containerinfra-labels
May 10, 2022
Merged

containerinfra: add a few missing labels fields#2377
mandre merged 2 commits intogophercloud:masterfrom
MrFreezeex:containerinfra-labels

Conversation

@MrFreezeex
Copy link
Copy Markdown
Contributor

@MrFreezeex MrFreezeex commented Apr 8, 2022 •
edited
Loading

Prior to a PR being reviewed, there needs to be a Github issue that the PR
addresses. Replace the brackets and text below with that issue number.

Fixes #2376

Links to the line numbers/files in the OpenStack source code that support the
code in this PR:

https://github.com/openstack/magnum/blob/20413d0c45dc7a6426072bd889bfb71c73fa62b1/magnum/api/controllers/v1/nodegroup.py#L129-L145
https://github.com/openstack/magnum/blob/20413d0c45dc7a6426072bd889bfb71c73fa62b1/magnum/api/controllers/v1/cluster.py#L187-L203

@MrFreezeex MrFreezeex changed the title Containerinfra labels containerinfra: add a few missing labels fields Apr 8, 2022
@coveralls
Copy link
Copy Markdown

coveralls commented Apr 8, 2022 •
edited
Loading

Coverage Status

Coverage remained the same at 79.99% when pulling cc57cde on MrFreezeex:containerinfra-labels into 9eb412d on gophercloud:master.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Apr 8, 2022

Build failed.

@mandre
Copy link
Copy Markdown
Contributor

mandre commented Apr 8, 2022

Thanks for the PR. You should normally have received a nice welcoming message for your first PR but our automation failed. I'm copying the content below:

Thank you for submitting your first PR! Be sure that we will be looking at it but keep in mind
this sometimes takes a while.
Please let the maintainers know if your PR has not got enough attention after a few days.
If any doubt, please consult our PR tutorial.

I'll review your PR in a minute.

@mandre
Copy link
Copy Markdown
Contributor

mandre commented Apr 8, 2022

This LGTM, however we don't have CI exercising your changes against a real openstack. We have yet to port the Magnum CI to github actions - I'd like we do that before merging your PR.

@mandre
Copy link
Copy Markdown
Contributor

mandre commented May 10, 2022

Hey @MrFreezeex, we've recently merged the change to enable CI for Magnum in #2379. Could you rebase this PR on top of master so that we see your change in action before we merge it?

MrFreezeex added 2 commits May 10, 2022 11:10
@MrFreezeex
containerinfra: add missing merge_labels in create nodegroup request
551f710 
Signed-off-by: Arthur Outhenin-Chalandre <arthur.outhenin-chalandre@cern.ch>
@MrFreezeex
containerinfra: add missing labels fields in result
cc57cde 
Signed-off-by: Arthur Outhenin-Chalandre <arthur.outhenin-chalandre@cern.ch>
@MrFreezeex MrFreezeex force-pushed the containerinfra-labels branch from 2ae729c to cc57cde Compare May 10, 2022 09:10
@MrFreezeex
Copy link
Copy Markdown
Contributor Author

MrFreezeex commented May 10, 2022

Hey @MrFreezeex, we've recently merged the change to enable CI for Magnum in #2379. Could you rebase this PR on top of master so that we see your change in action before we merge it?

Thanks for the great work, I just did that :D

mandre
mandre approved these changes May 10, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mandre mandre left a comment

Choose a reason for hiding this comment

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

Meh, this was pointless - we're still not exercising this new code in the acceptance tests. I couldn't get Magnum to deploy a cluster in CI and had to skip the tests that would have exercised this code :-/
Sorry for all the trouble, let's merge your PR as it looks good and the risk it breaks anything is rather low.

@mandre mandre merged commit 884fd39 into gophercloud:master May 10, 2022
@MrFreezeex MrFreezeex deleted the containerinfra-labels branch June 16, 2022 10:03
@MrFreezeex MrFreezeex mentioned this pull request Jun 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mandre mandre mandre 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.

Missing fields in containerinfra towards label supports

3 participants

@MrFreezeex @coveralls @mandre