Skip to content

cc_resolv_conf: fix typos#969

Merged
blackboxsw merged 3 commits into
canonical:mainfrom
sshedi:typo-fix
Aug 13, 2021
Merged

cc_resolv_conf: fix typos#969
blackboxsw merged 3 commits into
canonical:mainfrom
sshedi:typo-fix

Conversation

@sshedi
Copy link
Copy Markdown
Contributor

@sshedi sshedi commented Aug 11, 2021

No description provided.

@sshedi
Copy link
Copy Markdown
Contributor Author

sshedi commented Aug 11, 2021

@TheRealFalcon can we add a test to this? I have no idea how it sneaked in.

Comment thread cloudinit/config/cc_resolv_conf.py
Comment thread cloudinit/config/cc_resolv_conf.py
@sshedi sshedi changed the title cc_resolv_conf: fix typo cc_resolv_conf: fix typos Aug 11, 2021
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.

Ooph, thanks for the changes and the test here. Some of our modules still lack test coverage, and this particular one makes that very apparent.

One nit on the added test: It would be easier to read and help fix any potential failures if we had 5 separate tests, rather than checking the 5 conditions in one larger test. I won't block the PR on it, but it'd be a helpful small change.

Before I merge this, does photon need an update too? On the line I linked to, I noticed the key is resolv_conf_fn, but networkd.py is looking for resolved_conf_fn. Should that be updated too?

@sshedi
Copy link
Copy Markdown
Contributor Author

sshedi commented Aug 12, 2021

Ooph, thanks for the changes and the test here. Some of our modules still lack test coverage, and this particular one makes that very apparent.

One nit on the added test: It would be easier to read and help fix any potential failures if we had 5 separate tests, rather than checking the 5 conditions in one larger test. I won't block the PR on it, but it'd be a helpful small change.

Before I merge this, does photon need an update too? On the line I linked to, I noticed the key is resolv_conf_fn, but networkd.py is looking for resolved_conf_fn. Should that be updated too?

Oops. I will fix it. Also break the test into separate tests. Need sometime, I'm caught up with some other task. Is that okay?

@TheRealFalcon
Copy link
Copy Markdown
Contributor

Yep, no rush!

sshedi and others added 3 commits August 13, 2021 02:12
Add tests for cc_resolv_conf handler

Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
@blackboxsw blackboxsw merged commit 0404743 into canonical:main Aug 13, 2021
@sshedi sshedi deleted the typo-fix branch August 13, 2021 18:43
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.

3 participants