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

[master] Porting #52670 to master again #57637

Closed
wants to merge 5 commits into from

Conversation

OrangeDog
Copy link
Contributor

@OrangeDog OrangeDog commented Jun 11, 2020

Porting #52670 to master again, because the last one (#54572) got reverted by #56353.

When this is merged, vmware-archive/salt-pack#682 needs reverting.

@OrangeDog OrangeDog requested a review from a team as a code owner June 11, 2020 11:36
@ghost ghost requested review from xeacott and removed request for a team June 11, 2020 11:37
@dwoz dwoz added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jun 16, 2020
@sagetherage sagetherage requested review from waynew and removed request for xeacott December 3, 2020 17:34
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, boo, looks like needs pre-commit hooks 🙃

@waynew waynew removed Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases has-failing-test labels Dec 4, 2020
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@waynew waynew added the Aluminium Release Post Mg and Pre Si label Jan 7, 2021
@bryceml
Copy link
Contributor

bryceml commented Jan 12, 2021

don't ifup and ifdown set things up according to what's specified in /etc/network/interfaces as opposed to ip link set up which doesn't have anything to do with /etc/network/interfaces ?

This seems like it doesn't keep the same behavior at all if I understand it correctly.

It looks like the reason this was originally introduced was because ubuntu started using netplan by default. I'm pretty sure debian still uses ifupdown by default even on debian 10.

It would probably be more correct to do something like nmcli con up ... or nmcli con down ... if using network manager. I'm not sure how you would get the equivilant on netplan since it only really has netplan apply

Copy link
Contributor

@bryceml bryceml left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would break existing use of ifupdown on debian 10, this probably needs a much bigger PR to do the correct thing.

@OrangeDog
Copy link
Contributor Author

@bryceml if that's true, and nobody noticed on the previous two PRs, it should be tracked with #57541 and related.

@OrangeDog
Copy link
Contributor Author

It would probably be more correct to do something like nmcli con up ... or nmcli con down ... if using network manager. I'm not sure how you would get the equivilant on netplan since it only really has netplan apply

netplan is a higher-level config management system. The alternative to NetworkManager is systemd-networkd. I don't think there is any CLI other than ip. You just edit the config files and reload.

@sagetherage sagetherage removed the Aluminium Release Post Mg and Pre Si label Mar 1, 2021
@sagetherage
Copy link
Contributor

@OrangeDog we want to have a deep discussion on this since likely needs to be bigger than this PR and we need to discuss a broader solution. I am putting it into blocked for now, but I will schedule a planning meeting to dive deeper. I would love your input, I know our US meeting are difficult to attend, but I am open to other ideas the use of other platforms if that is helpful.

@sagetherage sagetherage added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Mar 1, 2021
@sagetherage sagetherage added this to the Blocked milestone Mar 1, 2021
@OrangeDog
Copy link
Contributor Author

I have no particular investment. I just noticed this PR has at least twice already been merged and then lost.

The concept is not difficult. You should be using the recommended commands on each of your supported operating systems and not relying on deprecated and removed packages. You need to work out what they are and then write that code. I think that ip is the default method on every linux OS version you support, but I may be mistaken.

@dwoz
Copy link
Contributor

dwoz commented Dec 10, 2023

Closing this due to inactivity. Anyone should feel free to re-open it if they want to see it through to the end in one release cycle.

@dwoz dwoz closed this Dec 10, 2023
@dwoz dwoz added help-wanted Community help is needed to resolve this Abandoned labels Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Abandoned help-wanted Community help is needed to resolve this Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants