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

netdog: refactor to prepare for upcoming additions #2330

Merged
merged 4 commits into from
Aug 12, 2022

Conversation

zmrow
Copy link
Contributor

@zmrow zmrow commented Aug 10, 2022

Issue number:
Related to #2204

Description of changes:
This refactor is in preparation for the upcoming additions related to static addressing.

Almost no code was changed minus the few clippy lints that are in the last commit. The majority of the code was moved around as is.

    netdog: Extract lease parsing functions to module
    netdog: refactor cli code into modules
    
    Since `main.rs` was starting to get large and unwieldy, and additional
    code is expected to be added, this commit refactors all the CLI code
    into separate files grouped under a `cli` module.
    netdog: implement the suggested clippy lints

Testing done:

  • Unit tests continue to pass
  • Build and run an aws-k8s-1.21 image successfully (current_ip, resolv.conf, primary_interface, and /etc/sysctl.d/90-primary_interface.conf all get properly written)
bash-5.1# cat /etc/sysctl.d/90-primary_interface.conf 
-net.ipv6.conf.eth0.accept_ra = 2
-net.ipv4.conf.eth0.rp_filter = 2
  • Build and run a metal-dev image successfully (net.toml correctly parsed and all above files written)
  • Manually run netdog's settings generator commands and observe proper output

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

Since `main.rs` was starting to get large and unwieldy, and additional
code is expected to be added, this commit refactors all the CLI code
into separate modules grouped under a `cli` module.
@zmrow
Copy link
Contributor Author

zmrow commented Aug 12, 2022

The above force push moves the parse_lease_info() function into an associated function of LeaseInfo; it makes more sense this way. :)

Copy link
Contributor

@bcressey bcressey left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -0,0 +1,32 @@
use super::{error, print_json, Result};
use crate::CURRENT_IP;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about moving the constants into some other module, like the parent?

Copy link
Contributor Author

@zmrow zmrow Aug 12, 2022

Choose a reason for hiding this comment

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

I spent more time than I'd like to admit playing and thinking about that... I ended up keeping them where they were since they get used in the CLI and other modules so I didn't really like them in cli/mod.rs and I also didn't really like the idea of making a top-level lib.rs just for constants.

Totally willing to entertain this though - perhaps there's an elegant way I just couldn't come up with! 😄

The `as_$THING` function signature is more idiomatic.  It also avoids a
clippy lint regarding `into_$THING` function signatures.
@zmrow
Copy link
Contributor Author

zmrow commented Aug 12, 2022

^ Adds a commit changing the into_wicked_interfaces() function signature into as_wicked_interfaces()... much better.

@zmrow zmrow merged commit f812645 into bottlerocket-os:develop Aug 12, 2022
@zmrow zmrow deleted the refactor-netdog branch August 12, 2022 20:50
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.

4 participants