-
Notifications
You must be signed in to change notification settings - Fork 869
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
add Openbsd support #147
add Openbsd support #147
Conversation
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.
🙋
1e32253
to
b4bf785
Compare
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 is quite the large PR! Congrats for putting this all together. I've left quite a few questions and concerns in there. I'm looking forward to adding some more BSD support.
3002935
to
e6f61b3
Compare
This requires reconciliation with master, so marking it as |
3a93a63
to
b15776b
Compare
@OddBloke Could you please remove the |
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.
Thanks for taking this on, it's much appreciated! At a high level, I think there's a fair amount of duplication between netbsd.py
and the new openbsd.py
. I'd like us to do some refactoring to reduce that duplication, as it will come back to bite us before too long.
Once that's happened, I'll give this a closer review (I didn't want to ask for changes to the copy/paste'd code, because that's how we end up with subtly out-of-sync parallel implementations).
cloudinit/distros/openbsd.py
Outdated
cmd.extend(args) | ||
|
||
pkglist = util.expand_package_list('%s-%s', pkgs) | ||
cmd.extend(pkglist) |
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 still needs to be resolved.
62de1e6
to
788ba0c
Compare
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.
Thanks for the changes. I've performed a more detailed review now, but overall this looks pretty good!
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.
Thanks for this iteration, this is looking a lot better! Still a few loose ends I've commented on inline.
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 LGTM now, thanks for all your hard work on it!
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.
Looks good, just a change and a question.
- tested on OpenBSD 6.6 - tested on OpenStack without config drive, and NoCloud with ISO config drive
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.
I'd like to clarify expectations on dhcp on all nics, and one name fix up (net -> open).
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.
OK. Thanks for all the work here. As we discussed in IRC, we'll defer the multi-nic DHCP fix to another PR since it needs to be fixed for all BSD variants.
Based on: #62