Skip to content

cloudinit.net refactor: apply_network_config_names#1388

Merged
TheRealFalcon merged 3 commits into
canonical:mainfrom
aciba90:1884602/cloudinit_net_refactor/apply_network_config_names
Apr 26, 2022
Merged

cloudinit.net refactor: apply_network_config_names#1388
TheRealFalcon merged 3 commits into
canonical:mainfrom
aciba90:1884602/cloudinit_net_refactor/apply_network_config_names

Conversation

@aciba90
Copy link
Copy Markdown
Contributor

@aciba90 aciba90 commented Apr 18, 2022

Proposed Commit Message

cloudinit.net refactor: apply_network_config_names

- Remove `apply_network_config_names` from `cloudinit.net` and `Distro` classes so it is only accessible via the networking attribute.

LP: #1884602

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

@aciba90 , thanks for this contribution.

I think the end goal of the net refactor (which has admittedly stalled), is to remove methods from the distro classes entirely and into the networking classes. So the call in stages.py would be self.distro.networking.apply_network_config_names(netcfg) instead.

In order to do that, we would also need implementations in the BSD networking class. The current implementations (e.g., 1 2 3 ) are unsupported or no-ops and would also need to be refactored into this networking class setup.

@aciba90
Copy link
Copy Markdown
Contributor Author

aciba90 commented Apr 19, 2022

Thank you for your comments.

I think the end goal of the net refactor (which has admittedly stalled), is to remove methods from the distro classes entirely and into the networking classes. So the call in stages.py would be self.distro.networking.apply_network_config_names(netcfg) instead.

I have removed those methods and made the change in stages.py.

In order to do that, we would also need implementations in the BSD networking class. The current implementations (e.g., 1 2 3 ) are unsupported or no-ops and would also need to be refactored into this networking class setup.

I have moved current implementations into BSDNetworking and FreeBSDNetworking.

@TheRealFalcon please let me know if there is something else to improve.

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the requested changes. One more minor change in where the new networking class lives, and I think we'll be good here.

Comment thread cloudinit/distros/freebsd.py Outdated
LOG = logging.getLogger(__name__)


class FreeBSDNetworking(BSDNetworking):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's put this class in networking.py. For now, I think it makes more sense there.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

)


class TestLinuxNetworkingApplyNetworkCfgNames:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice refactor of these tests! This is much nicer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

pytest.param("V2_CONFIG_NO_MAC", id="without_mac"),
],
)
def test_apply_v2_renames_skips_without_setname(self, config_attr: str):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: test_apply_v2_renames_skips_without_setname_or_mac

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@aciba90 aciba90 force-pushed the 1884602/cloudinit_net_refactor/apply_network_config_names branch from 288b377 to ef17769 Compare April 26, 2022 10:31
@aciba90
Copy link
Copy Markdown
Contributor Author

aciba90 commented Apr 26, 2022

Thanks for addressing the requested changes. One more minor change in where the new networking class lives, and I think we'll be good here.

Thanks for the comments. Resolved!

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I'm going to briefly close and then reopen this PR to trigger a Travis rebuild.

Comment thread cloudinit/distros/freebsd.py Outdated
LOG = logging.getLogger(__name__)


class FreeBSDNetworking(BSDNetworking):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's put this class into the networking.py. For now, I think it makes more sense there.

@TheRealFalcon TheRealFalcon reopened this Apr 26, 2022
@TheRealFalcon TheRealFalcon merged commit ce610eb into canonical:main Apr 26, 2022
@aciba90 aciba90 deleted the 1884602/cloudinit_net_refactor/apply_network_config_names branch April 26, 2022 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants