Skip to content

cc_update_etc_hosts: Use the distribution-defined path for the hosts file#983

Merged
TheRealFalcon merged 1 commit intocanonical:mainfrom
omniosorg:cc_hosts
Sep 2, 2021
Merged

cc_update_etc_hosts: Use the distribution-defined path for the hosts file#983
TheRealFalcon merged 1 commit intocanonical:mainfrom
omniosorg:cc_hosts

Conversation

@citrus-it
Copy link
Contributor

@citrus-it citrus-it commented Aug 17, 2021

Proposed Commit Message

cc_update_etc_hosts: Use the distribution-defined path for the hosts file

The distribution class has a property that specifies the location of
the system hosts file and this can be overridden in subclasses.
While the property is correctly used in distro.update_etc_hosts(), the
update_etc_hosts module does not use it and just assumes '/etc/hosts'

This fixes the module to use the distribution-specific property.

Additional Context

There are still places in the code where the path is assumed to be /etc/hosts but this
fixes the update_etc_hosts module.

Test Steps

I've been using this patch as part of my work to bring in support for the illumos family of operating systems, where the canonical hosts file is /etc/inet/hosts

2021-08-17 14:07:05,773 - util.py[DEBUG]: Cloud-init v. 21.2 running 'single' at Tue, 17 Aug 2021 14:07:05 +0000. Up 8903.657099246979 seconds.
2021-08-17 14:07:05,776 - stages.py[DEBUG]: Using distro class <class 'cloudinit.distros.omnios.Distro'>
2021-08-17 14:07:05,776 - stages.py[DEBUG]: Running module update_etc_hosts (<module 'cloudinit.config.cc_update_etc_hosts' from '/data/omnios-build/omniosorg/cloud-init/root/usr/lib/python3.9/site-packages/cloudinit/config/cc_update_etc_hosts.py'>) with frequency always
2021-08-17 14:07:05,777 - helpers.py[DEBUG]: Running config-update_etc_hosts using lock (<cloudinit.helpers.DummyLock object at 0xfffffc7fec541c40>)
2021-08-17 14:07:05,777 - util.py[DEBUG]: Reading from /etc/cloud/templates/hosts.illumos.tmpl (quiet=False)
2021-08-17 14:07:05,777 - util.py[DEBUG]: Read 783 bytes from /etc/cloud/templates/hosts.illumos.tmpl
2021-08-17 14:07:05,778 - templater.py[DEBUG]: Rendering content of '/etc/cloud/templates/hosts.illumos.tmpl' using renderer jinja
2021-08-17 14:07:05,785 - util.py[DEBUG]: Writing to /etc/inet/hosts - wb: [644] 455 bytes
2021-08-17 14:07:05,786 - util.py[DEBUG]: cloud-init mode 'single' took 0.191 seconds (0.15)

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

@TheRealFalcon
Copy link
Contributor

@citrus-it , thanks for the pull request!

I noticed you added yourself to the CLA signers file, but have you signed the actual CLA too? I'm not seeing your information there. If you did, can you provide the date that you signed it?

Other than that, what you have looks good. Can you also update the docstring in the module you changed? The docstring goes directly into our docs, so I think it would be good to replace references to /etc/hosts. Obviously we can't do this with the filename or module name, but I think we can update everything else there.

@citrus-it
Copy link
Contributor Author

noticed you added yourself to the CLA signers file, but have you signed the actual CLA too? I'm not seeing your information there. If you did, can you provide the date that you signed it?

I signed it on the 17th of August. I'll make the docstring changes you suggest.

@citrus-it citrus-it force-pushed the cc_hosts branch 2 times, most recently from 860d680 to ae316aa Compare August 30, 2021 22:29
@TheRealFalcon
Copy link
Contributor

Thanks for the updates. It looks good to me now.

I signed it on the 17th of August.

Unfortunately, I still have no record of the CLA. Do you mind trying again?

@citrus-it
Copy link
Contributor Author

Unfortunately, I still have no record of the CLA. Do you mind trying again?

I just signed it again.

Copy link
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, I see it now.

The branch needs a rebase before we'll be able to merge.

…file

The distribution class has a field that specifies the location of
the system hosts file and this can be overridden in subclasses.
While the field is correctly used in distro.update_etc_hosts(), the
update_etc_hosts module does not use it and just assumes '/etc/hosts'

This fixes the module to use the distribution-specific variable.
@citrus-it
Copy link
Contributor Author

The branch needs a rebase before we'll be able to merge.

done

@TheRealFalcon TheRealFalcon merged commit 7fe0f90 into canonical:main Sep 2, 2021
@TheRealFalcon
Copy link
Contributor

Thanks!

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