-
-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
cloud-init: 0.7.6 -> 0.7.9 + module improvements #23024
Conversation
@phile314, thanks for your PR! By analyzing the history of the files in this pull request, we identified @FRidh, @domenkozar and @madjar to be potential reviewers. |
I think a lot of cloud-init modules enabled by default don't work on nixos. Maybe it would be better to just set those that we use (this could be addressed later). |
I agree that the default config probably should be changed, but I don't know enough about cloud-init to do that myself. I only added the |
I don't understand why you need networkd? Could you precise a bit your use case? |
|
The example code I based this PR on had a |
c849f1d
to
4e0589d
Compare
I made the changes, any more comments? |
I didn't try it again but code looks good to me. |
@@ -19,10 +20,15 @@ in pythonPackages.buildPythonApplication rec { | |||
--replace /etc $out/etc \ | |||
--replace /lib/systemd $out/lib/systemd \ | |||
--replace 'self.init_system = ""' 'self.init_system = "systemd"' | |||
substituteInPlace cloudinit/sources/DataSourceAltCloud.py \ | |||
--replace /sbin/modprobe ''${kmod}/bin/modprobe \ | |||
--replace /sbin/udevadm ''${systemd}/bin/udevadm |
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.
will these quoted systemd refs ever be substituted?
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.
Oops, that's wrong. Nothing broke when I removed those, so doesn't seem to be necessary.
4e0589d
to
a0f4a72
Compare
ping Travis build error looks unrelated: error: while querying the derivation named ‘kiwix-0.9’: while evaluating the attribute ‘nativeBuildInputs’ of the derivation ‘kiwix-0.9’ at /tmp/nix-build-nixpkgs-tarball-17.09pre1234.abcdef.drv-0/nixpkgs/pkgs/applications/misc/kiwix/default.nix:38:3: cannot coerce a set to a string, at /tmp/nix-build-nixpkgs-tarball-17.09pre1234.abcdef.drv-0/nixpkgs/pkgs/applications/misc/kiwix/default.nix:38:3 |
ping |
1 similar comment
ping |
@@ -19,15 +20,17 @@ in pythonPackages.buildPythonApplication rec { | |||
--replace /etc $out/etc \ | |||
--replace /lib/systemd $out/lib/systemd \ | |||
--replace 'self.init_system = ""' 'self.init_system = "systemd"' | |||
|
|||
patchPhase |
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.
you call the patchPhase again from within the patchPhase?
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.
Yeah this won't work. Best to move all this into prePatch
.
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.
Why wouldn't it? It does work AFAICT (my understanding is that this will simply call the original patchPhase
script, won't it?). But prePatch
is probably cleaner.
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.
It definitely does work, but don't ask me why :-) Anyway, I will change it to use prePatch
.
The problem is that now the original patch phase script is not run and hence the |
Sorry, my bad, I only tested the build after that change but I forgot to actually run my test VM. You are right, the call to |
It appears that Also, resizing of the root disk doesn't work because |
The I think this PR should be merged as-is, the hostname problem can be solved in a separate PR. It's broken with the current cloud-init package in NixOS too anyway, so there is no regression here, only improvements. |
|
While you're at it, can you please add
as
That's what cloud images usually do (e.g. Ubuntu has the user |
@alexandergall I don't follow, what has the |
It seems logical to me to include it in this PR because |
Definitely deserves a second PR
…On Mon, Apr 24, 2017 at 8:17 AM Alexander Gall ***@***.***> wrote:
It seems logical to me to include it in this PR because cloud-init is all
about booting an image and it would save a PR. Feel free to ignore it if
you don't agree and I'll create a PR for it myself.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#23024 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAErrNQzXpo7SWAY827zhb_8tPMbo-3_ks5rzJLHgaJpZM4MGlwi>
.
|
Here is the problem with
I don't understand the reason for the replacements, since
works as expected. |
@alexandergall I don't understand the relationship between this PR and your last comment. Could you open another PR or issue to speak about the problem you mentioned? I personally really would like to see this PR merged since it improves the current cloud-init module. |
|
Sorry for having been a bit unresponsive here lately, has been a busy month... I can't really spend much more time on this PR. While having bugs in the cloud-init package is not so nice, I strongly suggest that we merge this if there are no regressions. As growpart is already broken in the old version, the growpart bug is not a regression. Please open a separate PR/Issue for any remaining bugs. @alexandergall While I can understand your point of view, this PR has already been open for a long time and I am very reluctant to put anything more into it. Delaying it anymore increases the risk of not merging this at all. I think the best way forward is to merge this improvement and make separate issues/PRs for any remaining issues. Also, this PR appears to be useful to other people so we shouldn't block it indefinitely. |
@phile314 sure, please go ahead. |
Ping. Can we merge this? |
Motivation for this change
Update cloud-init version and make the configuration an option. Also properly wait for online network. Based upon earlier work by @alexandergall .
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandbox
innix.conf
on non-NixOS)
nix-shell -p nox --run "nox-review wip"
./result/bin/
)