-
Notifications
You must be signed in to change notification settings - Fork 247
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
Fix configdrive handling #900
Conversation
/test-integration |
/test-integration |
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.
This would be slightly easier to review if the various fixes were in separate patches (within this PR).
/lgtm
Overall lgtm but I think the revised logic still isn't quite correct, because we'll only pass the default meta-data when one of (meta|network|user)Data is specified? Previously we only passed it when userData was specified, which I agree was incorrect, but I think the desired behavior is to always pass this metaData regardless of the user-provided data? |
I don't really know. It's a serious user-visible change. Could I follow-up with it, so that it can be reviewed separately (and reverted if things go south)? |
I'll bring it to the meeting today. If there are no objections, I'll update the PR. |
I've spotted a few errors with the configdrive handling: * We forgot to add configdrive to 'deploy failed' handler, so a repeated deployment will always happen without a configdrive. * ConfigDrive creation is conditional on UserData presence. This is incorrect, any field being populated should trigger configdrive. * There was no check on empty networkData, so we always tried to deserialize empty string with YAML (which works, but is incorrect). Create a new helper for configdrive, fix inline comments and cover configdrive creation with unit tests.
/test-integration |
/test-integration I've reverted the change to always build a configdrive after realizing it may break live ISO. We may need to skip configdrive creating for live ISO. |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elfosardo, hardys The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I've spotted a few errors with the configdrive handling:
deployment will always happen without a configdrive.
incorrect, any field being populated should trigger configdrive.
deserialize empty string with YAML (which works, but is incorrect).
Create a new helper for configdrive and cover it with unit tests.