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

Translate key names for OpenStack bond properties #5367

Merged
merged 2 commits into from
Jun 28, 2024
Merged

Translate key names for OpenStack bond properties #5367

merged 2 commits into from
Jun 28, 2024

Conversation

jcmoore3
Copy link
Contributor

@jcmoore3 jcmoore3 commented Jun 4, 2024 •
edited
Loading

Ensure that properties for bonded interfaces are translated properly from the OpenStack network_config.json format.

Fixes #5366

Proposed Commit Message

fix: Ensure properties for bonded interfaces are properly translated

There is a discrepancy between the properties key name formatting in
the OpenStack network_data.json and cloudinit network-config.json
specifications. Ensure `bond_` is translated to `bond-` when the
OpenStack configuration is parsed by cloudinit.

Fixes GH-5366

Additional Context

Test Steps

Checklist

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

aciba90
aciba90 requested changes Jun 6, 2024
View reviewed changes
Copy link
Contributor

@aciba90 aciba90 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 improving cloud-init. In addition to the inline comments, could we add some unit-tests under tests/unittests/sources/helpers/test_openstack.py covering this change?

cloudinit/sources/helpers/openstack.py Outdated
# cloudinit schema but 'bond_' in OpenStack
# network_data.json schema. Translate them to what
# is expected by cloudinit.
translated_key = re.sub(r'^(bond)_(.+)$', r'\1-\2', k)
Copy link
Contributor

Choose a reason for hiding this comment

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

Being this a simple split, could do something in the lines of the following and avoid using re to reduce complexity:

translated_key = k.split("bond_", 1)[-1]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Updated as requested to remove re.

@aciba90
Copy link
Contributor

aciba90 commented Jun 6, 2024

Could you sign the CLA, please?

https://docs.cloud-init.io/en/latest/development/index.html#prerequisites

@aciba90 aciba90 added the CLA not signed The submitter of the PR has not (yet) signed the CLA label Jun 6, 2024
@jcmoore3
Copy link
Contributor Author

jcmoore3 commented Jun 6, 2024

Could you sign the CLA, please?

https://docs.cloud-init.io/en/latest/development/index.html#prerequisites

I'm working to get approval for the CLA, I should have it signed near term.

@github-actions GitHub Actions
Copy link

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

@github-actions github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Jun 21, 2024
@holmanb
Copy link
Member

holmanb commented Jun 24, 2024

Could you sign the CLA, please?
https://docs.cloud-init.io/en/latest/development/index.html#prerequisites

I'm working to get approval for the CLA, I should have it signed near term.

@jcmoore3 Ping! Any updates on this? We can't merge any of your PRs until this happens. If you respond letting us know the status of this I will drop the stale-pr tag.

@jcmoore3
Copy link
Contributor Author

Apologies that the CLA is taking so long. This needs to go through my company's internal open source approval process. I do not anticipate any issues, it just takes time. I'm hoping to have final internal approval this week.

@holmanb holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label Jun 25, 2024
@jcmoore3
Copy link
Contributor Author

@holmanb - CLA signed, please let me know if anything else is required to get this moving. Change to github-cla-signers is in PR #5369.

@TheRealFalcon TheRealFalcon added CLA signed The submitter of the PR has signed the CLA and removed CLA not signed The submitter of the PR has not (yet) signed the CLA labels Jun 27, 2024
@aciba90
Copy link
Contributor

aciba90 commented Jun 28, 2024

Force-pushed to rebase main to include and fix the unittest from 12f1198 and 70f7e78.

aciba90
aciba90 approved these changes Jun 28, 2024
View reviewed changes
Copy link
Contributor

@aciba90 aciba90 left a comment

Choose a reason for hiding this comment

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

LGTM!

jcmoore3 and others added 2 commits June 28, 2024 11:59
@jcmoore3 @aciba90
fix: Ensure properties for bonded interfaces are properly translated (c…
15196cd 
…anonical#5367)

There is a discrepancy between the properties key name formatting in
the OpenStack network_data.json and cloudinit network-config.json
specifications. Ensure `bond_` is translated to `bond-` when the
OpenStack configuration is parsed by cloudinit.

Fixes canonicalGH-5366

Co-authored-by: Alberto Contreras <[email protected]>
@aciba90
test: fix unit test openstack vlan mac_address (canonical#5367)
0f4082e 
After 70f7e78, mac_address is not
rendered for in openstack for network config v1. Fix the unit test to
reflect that.
@aciba90
Copy link
Contributor

aciba90 commented Jun 28, 2024

Force-pushed to fix commit message.

@aciba90 aciba90 merged commit debafbc into canonical:main Jun 28, 2024
23 checks passed
aciba90 added a commit that referenced this pull request Jun 28, 2024
@jcmoore3 @aciba90
fix: Ensure properties for bonded interfaces are properly translated (#…
371b236 
…5367)

There is a discrepancy between the properties key name formatting in
the OpenStack network_data.json and cloudinit network-config.json
specifications. Ensure `bond_` is translated to `bond-` when the
OpenStack configuration is parsed by cloudinit.

Fixes GH-5366

Co-authored-by: Alberto Contreras <[email protected]>
holmanb pushed a commit that referenced this pull request Jun 28, 2024
@jcmoore3 @aciba90 @holmanb
fix: Ensure properties for bonded interfaces are properly translated (#…
a7b21c7 
…5367)

There is a discrepancy between the properties key name formatting in
the OpenStack network_data.json and cloudinit network-config.json
specifications. Ensure `bond_` is translated to `bond-` when the
OpenStack configuration is parsed by cloudinit.

Fixes GH-5366

Co-authored-by: Alberto Contreras <[email protected]>
holmanb pushed a commit that referenced this pull request Jun 28, 2024
@aciba90 @holmanb
test: fix unit test openstack vlan mac_address (#5367)
20d1cbf 
After 70f7e78, mac_address is not
rendered for in openstack for network config v1. Fix the unit test to
reflect that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aciba90 aciba90 aciba90 approved these changes

Assignees
No one assigned
Labels
CLA signed The submitter of the PR has signed the CLA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cloud config schema error for bond properties
4 participants
@jcmoore3 @aciba90 @holmanb @TheRealFalcon