The proposal to merge ~lamoura/curtin:disable-networking-config into
curtin:master has been updated.
Status: Approved => Merged
For more details, see:
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
--
Your team curtin developers is subscribed to branch curtin:master.
The proposal to merge ~lamoura/curtin:disable-networking-config into
curtin:master has been updated.
Status: Needs review => Approved
For more details, see:
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
--
Your team curtin developers is subscribed to branch curtin:mas
The proposal to merge ~lamoura/curtin:disable-networking-config into
curtin:master has been updated.
Commit message changed to:
net_meta: add disabled mode to skip writing any network config
Curtin's install command invokes 'net-meta auto' by default
which will handle provided network config or
Review: Approve
Thanks!
--
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/
Review: Approve continuous-integration
PASSED: Continuous integration, rev:45e02af4f40d8694897ac4a27237fc66a92d9874
https://jenkins.ubuntu.com/server/job/curtin-ci/122/
Executed test runs:
SUCCESS:
https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/122/
SUCCESS:
https://je
raharper, I have updated the docstrings
Diff comments:
> diff --git a/tests/vmtests/test_network_disabled.py
> b/tests/vmtests/test_network_disabled.py
> new file mode 100644
> index 000..cf55030
> --- /dev/null
> +++ b/tests/vmtests/test_network_disabled.py
> @@ -0,0 +1,73 @@
> +# This file
Review: Approve continuous-integration
PASSED: Continuous integration, rev:acc75e2e528df66ea4ae8d9134a7adb6d14b2d63
https://jenkins.ubuntu.com/server/job/curtin-ci/120/
Executed test runs:
SUCCESS:
https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/120/
SUCCESS:
https://je
raharper, I have updated the code with the test refactoring. All network
disable tests are now in the same file.
--
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~cur
On Wed, May 20, 2020 at 1:43 PM Lucas Albuquerque Medeiros de Moura <
lucas.mo...@canonical.com> wrote:
> Wait, I think I missed something for these scenarios:
>
> network: {config: disabled}
> network: {config: disabled, version=1}
>
> We should not create the /etc/cloud/cloud.cfg.d/50-curtin-n
Wait, I think I missed something for these scenarios:
network: {config: disabled}
network: {config: disabled, version=1}
We should not create the /etc/cloud/cloud.cfg.d/50-curtin-networking.cfg file
for them ?
I thought that they were valid cloudinit configs and we should propagate them.
Rig
For the three scenarios:
net-meta mode=disabled
network: {config: disabled}
network: {config: disabled, version=1}
We need to check that in the target system, we do not find
/etc/cloud/cloud.cfg.d/50-curtin-networking.cfg
So, let's put that unittest in its own base class and then
build each scen
The TestNetMetaDisabledModeBaseTestsAbs overrides some of tests that the
network disable config should use. For example, this class overrides the test:
test_cloudinit_network_passthrough
Which is a test that should be performed for the disable cloud config tests.
But I can move this override to
Review: Needs Information
Thanks, one more question on the vmtests; it's not clear to me why we've got
network-disabled tests in test_network.py rather than both of those classes in
test_network_disabled.py and have the two different config versions subclass
from your main TestNetMetaDisabled a
Review: Approve continuous-integration
PASSED: Continuous integration, rev:1bf73abf551d8bdcfa026f943f0d2403544d3af0
https://jenkins.ubuntu.com/server/job/curtin-ci/117/
Executed test runs:
SUCCESS:
https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/117/
SUCCESS:
https://je
This MP should get the CI going again:
https://code.launchpad.net/~legovini/curtin/+git/curtin/+merge/384245
Background info: the vmtests run on the MAAS images, which are automatically
synced as part of vmtest Jenkins jobs when needed. (The logic is slightly more
complex, but doesn't matter he
Review: Needs Fixing continuous-integration
FAILED: Continuous integration, rev:1bf73abf551d8bdcfa026f943f0d2403544d3af0
https://jenkins.ubuntu.com/server/job/curtin-ci/114/
Executed test runs:
SUCCESS:
https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/114/
FAILURE:
https
raharper, I have fixed the unittest. Also, both of these tests are currently
skipped for the disable config scenarios, for both config with and without
version key.
Diff comments:
> diff --git a/tests/unittests/test_commands_net_meta.py
> b/tests/unittests/test_commands_net_meta.py
> new file
Review: Needs Fixing
I see, unittests around vmtest results. For those tests we can mark them a
skiptests; we don't expect to verify the network config of the target machine
when it's been disabled.
This looks really good. Just a couple more small points inline.
Diff comments:
> diff --git
Review: Needs Fixing continuous-integration
FAILED: Continuous integration, rev:688541726ab1137dcee06ae3999749f456ad0a68
https://jenkins.ubuntu.com/server/job/curtin-ci/113/
Executed test runs:
FAILURE:
https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/113/
FAILURE:
https
rahaper, I have updated the code with your suggestions.
I have also added in the comments the reason why I believe those tests need to
be skipped.
They are;
test_etc_resolvconf
test_ip_output
With this solution, I have not used the property yet. But you think we can
still benefit from it, just
Diff comments:
> diff --git a/curtin/commands/apply_net.py b/curtin/commands/apply_net.py
> index ddc5056..5f1bc23 100644
> --- a/curtin/commands/apply_net.py
> +++ b/curtin/commands/apply_net.py
> @@ -99,6 +99,11 @@ def apply_net(target, network_state=None,
> network_config=None):
>
Review: Needs Fixing
Thanks for updating. I've added some more inline comments.
>From your last comments:
> but I had to skip some tests that directly used the network state. However
> this is not the case for the disabled config test with a version key. Maybe
> we could always return a nul
Review: Approve continuous-integration
PASSED: Continuous integration, rev:072f0f151bf9b413879db512396977c00e25f15f
https://jenkins.ubuntu.com/server/job/curtin-ci/108/
Executed test runs:
SUCCESS:
https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/108/
SUCCESS:
https://je
raharper, I have addressed the proposed changes, Just one doubt about the
differences in testing between the disabled config with a vesion key and
without it
Diff comments:
> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 4afe00c..e0498f6 100644
> --- a/curtin/
Thanks, a couple more comments inline.
Diff comments:
> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 4afe00c..e0498f6 100644
> --- a/curtin/commands/curthooks.py
> +++ b/curtin/commands/curthooks.py
> @@ -1155,7 +1155,7 @@ def detect_required_packages(cfg,
> o
Review: Approve continuous-integration
PASSED: Continuous integration, rev:b887db1007ed30af5521b20a8fbd95bb31a468c6
https://jenkins.ubuntu.com/server/job/curtin-ci/107/
Executed test runs:
SUCCESS:
https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/107/
SUCCESS:
https://je
raharper, I have update the PR with the proposed changes and addressed some
concerns related to the version key on the network config
Diff comments:
> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 4afe00c..e0498f6 100644
> --- a/curtin/commands/curthooks.py
> +
Diff comments:
> diff --git a/curtin/commands/apply_net.py b/curtin/commands/apply_net.py
> index ddc5056..8e5596b 100644
> --- a/curtin/commands/apply_net.py
> +++ b/curtin/commands/apply_net.py
> @@ -82,22 +82,23 @@ def apply_net(target, network_state=None,
> network_config=None):
> eli
Review: Needs Fixing
Thanks for the refactor. I think we can simplify a few things. Comments
inline.
Diff comments:
> diff --git a/curtin/commands/apply_net.py b/curtin/commands/apply_net.py
> index ddc5056..8e5596b 100644
> --- a/curtin/commands/apply_net.py
> +++ b/curtin/commands/apply_net
Review: Approve continuous-integration
PASSED: Continuous integration, rev:1b3e7dc369889950421bde34ba519ac860aa2a86
https://jenkins.ubuntu.com/server/job/curtin-ci/103/
Executed test runs:
SUCCESS:
https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/103/
SUCCESS:
https://je
Review: Needs Fixing continuous-integration
FAILED: Continuous integration, rev:d690b9e7263b80b63328f66ca7943b3d441407bc
https://jenkins.ubuntu.com/server/job/curtin-ci/102/
Executed test runs:
FAILURE:
https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/102/
FAILURE:
https
raharper I have addressed the changes you commented. I have created the
disabled option on the net_meta module and created a VMTest for this command.
I have also created two new VMTests for the {"config": "disabled"} possibility
as well. For the case of a network disabled config without a versio
Review: Needs Fixing continuous-integration
FAILED: Continuous integration, rev:714e3b8a42af33a7b347c1206f04c708b5b73ee8
https://jenkins.ubuntu.com/server/job/curtin-ci/101/
Executed test runs:
FAILURE:
https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/101/
FAILURE:
https
I moved the description to the commit message field, so hopefully we won't get
flooded :)
--
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : c
The proposal to merge ~lamoura/curtin:disable-networking-config into
curtin:master has been updated.
Description changed to:
For more details, see:
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
--
Your team curtin developers is subscribed to branch curtin:master.
--
The proposal to merge ~lamoura/curtin:disable-networking-config into
curtin:master has been updated.
Description changed to:
For more details, see:
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
--
Your team curtin developers is subscribed to branch curtin:master.
--
M
The proposal to merge ~lamoura/curtin:disable-networking-config into
curtin:master has been updated.
Commit message changed to:
Currently, there is no easy way for curtin to not write any network_config on
state/network_config. This file is then used by the curthooks to apply network
parameter
Review: Needs Fixing continuous-integration
No commit message was specified in the merge proposal. Click on the following
link and set the commit message (if you want jenkins to rebuild you need to
trigger it yourself):
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-
Review: Needs Fixing continuous-integration
No commit message was specified in the merge proposal. Click on the following
link and set the commit message (if you want jenkins to rebuild you need to
trigger it yourself):
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-
Review: Needs Fixing continuous-integration
No commit message was specified in the merge proposal. Click on the following
link and set the commit message (if you want jenkins to rebuild you need to
trigger it yourself):
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-
- CI loop due to the missing commit message
- Failures due to the new flake8
--
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : curtin-dev@lis
Review: Needs Fixing continuous-integration
No commit message was specified in the merge proposal. Click on the following
link and set the commit message (if you want jenkins to rebuild you need to
trigger it yourself):
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-
Review: Needs Fixing continuous-integration
No commit message was specified in the merge proposal. Click on the following
link and set the commit message (if you want jenkins to rebuild you need to
trigger it yourself):
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-
Review: Needs Fixing continuous-integration
No commit message was specified in the merge proposal. Click on the following
link and set the commit message (if you want jenkins to rebuild you need to
trigger it yourself):
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-
Review: Needs Fixing continuous-integration
No commit message was specified in the merge proposal. Click on the following
link and set the commit message (if you want jenkins to rebuild you need to
trigger it yourself):
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-
Review: Needs Fixing continuous-integration
No commit message was specified in the merge proposal. Click on the following
link and set the commit message (if you want jenkins to rebuild you need to
trigger it yourself):
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-
Review: Needs Fixing continuous-integration
No commit message was specified in the merge proposal. Click on the following
link and set the commit message (if you want jenkins to rebuild you need to
trigger it yourself):
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-
Review: Needs Fixing continuous-integration
No commit message was specified in the merge proposal. Click on the following
link and set the commit message (if you want jenkins to rebuild you need to
trigger it yourself):
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-
Review: Needs Fixing continuous-integration
No commit message was specified in the merge proposal. Click on the following
link and set the commit message (if you want jenkins to rebuild you need to
trigger it yourself):
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-
Thanks for the review Ryan :)
I do recall that you commented about the net_meta options, but I misunderstood
what you said for modifying the auto command to not write the network_config
and not creating a new option.
I will address all of the proposed changes and also thanks for the broader
co
Review: Needs Fixing
Thanks for getting something started. The origin of this scenario comes from
the Subiquity installer which handles writing a network configuration into the
target outside of curtin and cloud-init. In this scenario, subiquity provides
*no* network config at all to curtin,
Lucas Albuquerque Medeiros de Moura has proposed merging
~lamoura/curtin:disable-networking-config into curtin:master.
Requested reviews:
curtin developers (curtin-dev)
For more details, see:
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Currently, there is no easy way
52 matches
Mail list logo
