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

distros/photon.py: refactor hostname handling #958

Merged
merged 3 commits into from
Aug 9, 2021

Conversation

sshedi
Copy link
Contributor

@sshedi sshedi commented Aug 3, 2021

Signed-off-by: Shreenidhi Shedi sshedi@vmware.com

@sshedi
Copy link
Contributor Author

sshedi commented Aug 3, 2021

Hi @TheRealFalcon - can you please review this change? You suggested to use this in my very first PR, at that time I took a part of hostname related chnages from RHEL distro code. Now I have tried to make it better.

Copy link
Member

@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.

Looks good to me!

I added two small minor suggestions inline, but they won't block merge.

cloudinit/distros/photon.py Outdated Show resolved Hide resolved
cloudinit/distros/photon.py Outdated Show resolved Hide resolved
LOG.debug('Attempting to run: %s', cmd)
try:
(out, err) = subp.subp(cmd, capture=capture)
if err:
LOG.warning('Running %s resulted in stderr output: %s',
cmd, err)
return True, out, err
return True, out, err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the return values here.
If a command fails, this will return True else False.

cloudinit/distros/photon.py Outdated Show resolved Hide resolved
@sshedi
Copy link
Contributor Author

sshedi commented Aug 6, 2021

@TheRealFalcon - I have added networkd activator in this PR now. Please take a look.
If you want me to send a different PR for network activator changes, I will do so.

Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
Copy link
Member

@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.

Looks good, thanks!

@TheRealFalcon TheRealFalcon merged commit 049d62b into canonical:main Aug 9, 2021
@sshedi
Copy link
Contributor Author

sshedi commented Aug 9, 2021

Welcome and thank you too.

TheRealFalcon pushed a commit to TheRealFalcon/cloud-init that referenced this pull request Aug 10, 2021
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.

None yet

2 participants