load all substitutions from etc

[evgeni]

On some distributions (like CentOS Stream and Oracle Linux), we need more substitutions to be able to load repositories properly.

DNF has a helper for that: conf.substitutions.update_from_etc.

On pure DNF distributions, calling this should be sufficient. On EL7, where the primary tool is YUM, DNF does not load vars from /etc/yum, only from /etc/dnf, so we have to help it a bit and explicitly try to load releasever from /etc/yum.
(DNF since 4.2.15 does also load substitutions from /etc/yum, but EL7 ships with 4.0.x)

[github-actions]

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

• review please @oamg/developers to notify leapp developers of the review request
• /packit copr-build to submit a public copr build using packit

Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build. If you need a different version of leapp from PR#42, use /packit test oamg/leapp#42

It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported, beaker-minimal and kernel-rt, both can be used to be run on all upgrade paths or just a couple of specific ones.
To launch on-demand tests with packit:

• /packit test --labels kernel-rt to schedule kernel-rt tests set for all upgrade paths
• /packit test --labels beaker-minimal-8.9to9.3,kernel-rt-8.9to9.3 to schedule kernel-rt and beaker-minimal test sets for 8.9->9.3 upgrade path

[Deprecated] To launch on-demand regression testing public members of oamg organization can leave the following comment:

• /rerun to schedule basic regression tests using this pr build and latest upstream leapp build as artifacts
• /rerun 42 to schedule basic regression tests using this pr build and leapp*PR42* as artifacts
• /rerun-sst to schedule sst tests using this pr build and latest upstream leapp build as artifacts
• /rerun-sst 42 to schedule sst tests using this pr build and leapp*PR42* as artifacts

Please open ticket in case you experience technical problem with the CI. (RH internal only)

Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra.

[evgeni]

review please @oamg/developers

[abadger]

I would put the releasever_path setting inside of the condition to show that it is only a part of the code needed to handle EL7:

Suggested change

	# dnf on EL7 doesn't load vars from /etc/yum, so we need to help it a bit
	releasever_path = '/etc/yum/vars/releasever'
	if get_source_major_version() == '7' and os.path.exists(releasever_path):
	with open(releasever_path) as releasever_file:
	# dnf on EL7 doesn't load vars from /etc/yum, so we need to help it a bit
	if get_source_major_version() == '7' and os.path.exists(releasever_path):
	releasever_path = '/etc/yum/vars/releasever'
	with open(releasever_path) as releasever_file:

A general note: using a try: except: around open() and passing if the file does not exist is resistant to race conditions whereas checking file existence first and then operating on the file is not so I generally prefer the try: except: strategy. However, I see (1) that we use the file existence check strategy all over the code right now and (2) this specific case is unlikely to have the file deleted out from under us. So, something to keep in mind but I won't require that change to be made.

[evgeni]

I can't call os.path.exists(releasever_path) before assigning releasever_path = '/etc/yum/vars/releasever' ;-)

The logic I wanted to keep is "if on EL7 and the file exists, load it, in all other cases use get_source_major_version.

I could rewrite this to:

conf.substitutions['releasever'] = get_source_major_version()
if get_source_major_version() == '7':
  releasever_path = '/etc/yum/vars/releasever'
  if os.path.exists(releasever_path):
    with open(releasever_path) as releasever_file:
      conf.substitutions['releasever'] = releasever_file.read().strip()

# load all substitutions from etc
conf.substitutions.update_from_etc('/')

But it doesn't make the code much more readable, IMHO.

[evgeni]

Or even

conf.substitutions['releasever'] = get_source_major_version()

if get_source_major_version() == '7':
  try:
    with open('/etc/yum/vars/releasever') as releasever_file:
      conf.substitutions['releasever'] = releasever_file.read().strip()
  except:
    pass

# load all substitutions from etc
conf.substitutions.update_from_etc('/')

This might be better readable?

[evgeni]

Done this now. Let me know what you think.

[abadger]

+1

[abadger]

And yeah, I got ahead of myself thinking about both changing to try: except: and moving the assignment but then only making one of those a suggestion.

[abadger]

I don't know the dnf and yum apis intimately but the reasoning here makes sense and I've looked at the relevant code in dnf.conf.substitutions.update_from_etc() to check that it takes care of this case in EL8.

The only changes I asked for are cosmetic so I'm approving this. You can add my suggestion or let me know you'd rather not and then this can be merged.

[abadger]

Looks good

[abadger]

Thanks @evgeni !