Skip to content
This repository has been archived by the owner on Aug 23, 2021. It is now read-only.

Add a SPEC file for EL8 #46

Closed
wants to merge 2 commits into from
Closed

Conversation

varesa
Copy link

@varesa varesa commented Jul 16, 2020

Add a SPEC file which can be used to build RPMs for CentOS and RHEL 8.

Based on request from @akutz to upstream changes in https://github.com/varesa/cloud-init-vmware-guestinfo/tree/el8 (19ee229)
This PR drops the README change (not relevant) and squashes the other two commits into one (with the other one being a minor fix)

Verified that this slightly modified version succesfully builds an RPM using akutz@35f0351 and that the resulting package installs in a centos:8 container

EDIT: Appended to include akutz@35f0351 as well

@varesa
Copy link
Author

varesa commented Jul 16, 2020

Though now I checked that the master branch still has the "issue" for which I created the fork from a past revision in the first place:
the dependency on deepmerge (see also #12)

While the package installs, the python module does not load:

[root@04781f179a17 /]# python3 /usr/lib/python3.6/site-packages/cloudinit/sources/DataSourceVMwareGuestInfo.py
Traceback (most recent call last):
  File "/usr/lib/python3.6/site-packages/cloudinit/sources/DataSourceVMwareGuestInfo.py", line 39, in <module>
    from deepmerge import always_merger
ModuleNotFoundError: No module named 'deepmerge'

Since deepmerge still doesn't seem to be part of the EL8 standard repositories, there seem to be really three options:

  • Remove dependency on the library
  • Vendor the library inside the package (not sure if this is really an acceptable practice with rpm/python)
  • Maintain an RPM of the deepmerge library

rpm.el8.spec Outdated Show resolved Hide resolved
rpm.el8.spec Show resolved Hide resolved
@akutz
Copy link
Contributor

akutz commented Jul 16, 2020

Though now I checked that the master branch still has the "issue" for which I created the fork from a past revision in the first place:
the dependency on deepmerge (see also #12)

While the package installs, the python module does not load:

[root@04781f179a17 /]# python3 /usr/lib/python3.6/site-packages/cloudinit/sources/DataSourceVMwareGuestInfo.py
Traceback (most recent call last):
  File "/usr/lib/python3.6/site-packages/cloudinit/sources/DataSourceVMwareGuestInfo.py", line 39, in <module>
    from deepmerge import always_merger
ModuleNotFoundError: No module named 'deepmerge'

Since deepmerge still doesn't seem to be part of the EL8 standard repositories, there seem to be really three options:

  • Remove dependency on the library
  • Vendor the library inside the package (not sure if this is really an acceptable practice with rpm/python)
  • Maintain an RPM of the deepmerge library

FWIW, this is why I prefer the curl based install, it installs the lib via pip.

Anyway, I originally did not use deep merge, so I know it's possible not to use it. If you want to open a PR to remove its use, that's fine. Or I can do it. But it has to be backwards compatible with the current format of the metadata, and more importantly, the instance data file.

This patch updates the Makefile and Dockerfile.rpmbuild to support
building the RPM for CentOS 8.

This patch also includes a few, small addendums required to make the
previous patches work for CentOS 8.
@varesa
Copy link
Author

varesa commented Jul 17, 2020

FWIW, this is why I prefer the curl based install, it installs the lib via pip.

That is probably the easy way, but does not work too nicely with disconnected install (no direct access to internet), like if a company uses internal package repositories/mirrors for software. Bypassing the OS package manager also makes it harder to identify installed software (for security/compliance reasons) or update in case of security issues.

Technically it is possible to use pre/post scripts in an RPM to download libraries or do pretty much anything but that is generally a really bad idea and breaks some of the above mentioned use cases.

Anyway, I originally did not use deep merge, so I know it's possible not to use it. If you want to open a PR to remove its use, that's fine. Or I can do it. But it has to be backwards compatible with the current format of the metadata, and more importantly, the instance data file.

If the library is easy enough to replace with an in-repo implementation, that would in my opinion be the cleanest way to solve the issue. I haven't looked into the details of how it is used or what it would take to replace it so at least for now I won't promise to make that change.

I noticed that there was also some interesting in moving this provider into the cloud-init core and these dependencies were also blocking that. If that is an approach you're interested in going for in the future, removing deepmerge could be a step towards that as well

@akutz
Copy link
Contributor

akutz commented Jul 17, 2020

Do you think we should block this PR until the deep merge issue is resolved? I am fine merging this as-is and then not publishing some RPM until the issue is resolved, or updating the README to include the proviso. What do you think?

@varesa
Copy link
Author

varesa commented Jul 22, 2020

I don't really have a strong opinion towards either option. While the RPM SPEC alone is not particularly useful, including it doesn't seem too harmful either.

@mtronrd
Copy link

mtronrd commented Aug 13, 2020

I think maybe the best option here is to use util.mergemanydict(). This removes the deepmerge dependency and brings the module in line with many of the built in datasource modules.

@akutz
Copy link
Contributor

akutz commented Aug 11, 2021

This issue is being closed because this datasource has been merged into cloud-init (canonical/cloud-init#953):

Component Source Tests
Datasource DataSourceVMware.py test_vmware.py
Identification ds-identify test_ds_identify.py
Documentation vmware.rst

In order to participate in the growth of this datasource moving forward, please:

Once again, many thanks to the wonderful community that has grown around this datasource, and I look forward to seeing everyone in the new cloud-init forums!

@akutz akutz closed this Aug 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants