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.

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

ncm-network: mirror "old" bootproto behaviour in nmstate #1653

Closed
wants to merge 1 commit into from

Conversation

wdpypere
Copy link
Contributor

@wdpypere wdpypere commented Feb 3, 2024

I spent a good time figuring out why nmstate.pm was not changing IP's. The code of nmstate is:

    if (defined($eth_bootproto)) {
        if ($eth_bootproto eq 'static') {

In our templates we don't set bootproto.

but if I look in network.pm it does:

    # set the bootprotocol
    &$makeline('bootproto', def => 'static');

    my $bootproto = $iface->{bootproto} || 'static';

So I think nmstate.pm should mimic that behavior.

@wdpypere
Copy link
Contributor Author

wdpypere commented Feb 3, 2024

tests fail because other parts of the code (vlans and bridges) seem to assume bootproto to be unset. So this would need more fixing. But I do think nmstate should mimic network.pm, to match admin expectations.

@aka7
Copy link
Contributor

aka7 commented Feb 3, 2024

tests fail because other parts of the code (vlans and bridges) seem to assume bootproto to be unset. So this would need >>more fixing. But I do think nmstate should mimic network.pm, to match admin expectations.

this PR should fix the failures you are seeing.
#1647

@wdpypere
Copy link
Contributor Author

wdpypere commented Feb 4, 2024

ah, I missed that. I'll close this PR. Thanks @aka7

@wdpypere wdpypere closed this Feb 4, 2024
@aka7
Copy link
Contributor

aka7 commented Feb 4, 2024

@wdpypere I think you will stiill need your PR if you want bootproto to be static by default.
but I guess either after mine is merged and you rebase? or I can add it with that pr?

@aka7
Copy link
Contributor

aka7 commented Feb 5, 2024

@wdpypere I think you will stiill need your PR if you want bootproto to be static by default. but I guess either after mine is merged and you rebase? or I can add it with that pr?

I've made changes to the pr to include your suggestion and as a result made that part of the code a bit more simpler. Please pull this in to test if you like. #1647 @wdpypere and a review would be welcomed.

@wdpypere
Copy link
Contributor Author

wdpypere commented Feb 5, 2024

@aka7 thanks for adding to the other PR. Ill read it, but I don't think you want my review, I'm very bad at Perl. :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants