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

dnfbase: Fix substitutions #1380

Merged
merged 2 commits into from
Feb 16, 2024
Merged

dnfbase: Fix substitutions #1380

merged 2 commits into from
Feb 16, 2024

Conversation

bcl
Copy link
Contributor

@bcl bcl commented Feb 14, 2024

Branching for Fedora 40 revealed a problem with using substitutions in the GPG key name, @AdamWill fixed this by setting them on the Base object which appears to be the correct way to do it.

Remove the per-repo kludge and set releasever and basearch on the Base variables so they are used everywhere.

Branching for Fedora 40 revealed a problem with using substitutions in
the GPG key name, @AdamWill fixed this by setting them on the Base
object which appears to be the correct way to do it.

Remove the per-repo kludge and set releasever and basearch on the Base
variables so they are used everywhere.
@bcl
Copy link
Contributor Author

bcl commented Feb 14, 2024

Fixes #1379

@AdamWill
Copy link
Contributor

AdamWill commented Feb 14, 2024

I am not actually sure that setting basearch this way works. At least, the two bits of code in dnf5 itself that set releasever this way - here and here - do not set basearch the same way, which seems like a warning flag. I'll try it out and see what happens.

@AdamWill
Copy link
Contributor

AdamWill commented Feb 14, 2024

well, I tried it this way, and it seems to be working. I do kinda feel like I want to ask dnf5 folks what exactly is going on here and if this is the right way, though - @jan-kolarik @m-blaha ? edit: for more context, my original PR was #1379 which just added this approach for releasever but kept the substitutions , @bcl extended it to this.

@jan-kolarik
Copy link

well, I tried it this way, and it seems to be working. I do kinda feel like I want to ask dnf5 folks what exactly is going on here and if this is the right way, though - @jan-kolarik @m-blaha ? edit: for more context, my original PR was #1379 which just added this approach for releasever but kept the substitutions , @bcl extended it to this.

The current situation looks as expected from the DNF point of view. You set up the variables, and then when the repositories are created, they are substituted with the real values in sack.create_repos_from_reposdir().

However, I couldn't find proper documentation for this in the current API docs, so I've added it to our existing issue, and we'll strive to provide better documentation soon.

@coveralls
Copy link

coveralls commented Feb 15, 2024

Pull Request Test Coverage Report for Build 7933079822

Details

  • 0 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 43.521%

Totals Coverage Status
Change from base Build 7808681794: -0.02%
Covered Lines: 1614
Relevant Lines: 3500

💛 - Coveralls

@AdamWill
Copy link
Contributor

@jan-kolarik thanks! Just to be clear - when you say "The current situation looks as expected from the DNF point of view", do you mean the "current situation" as in "the changes in this pull request are correct"?

@jan-kolarik
Copy link

@jan-kolarik thanks! Just to be clear - when you say "The current situation looks as expected from the DNF point of view", do you mean the "current situation" as in "the changes in this pull request are correct"?

Yes, sorry for my cumbersome expressions 😛

@@ -153,6 +153,10 @@ def sanitize_repo(repo):
log.info("Using %s for module_platform_id", platform_id)
conf.module_platform_id = platform_id

# Set variables used for substitutions
dnfbase.get_vars().set("releasever", releasever)
dnfbase.get_vars().set("basearch", basearch)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a good place for setting vars.

@j-mracek
Copy link
Contributor

I am not actually sure that setting basearch this way works. At least, the two bits of code in dnf5 itself that set releasever this way - here and here - do not set basearch the same way, which seems like a warning flag. I'll try it out and see what happens.

The links refers to a commandline overrides from options. The patch looks OK.

@bcl
Copy link
Contributor Author

bcl commented Feb 16, 2024

Can't figure out what's up with github. The tests pass, and I updated the checkout version like it suggested but it still says waiting for status. Merging it anyway.

@bcl bcl merged commit 55f8077 into weldr:master Feb 16, 2024
1 of 2 checks passed
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.

5 participants