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

properly load inherited foreman settings #419

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Sep 2, 2024

Fixes: 0ee482d

settings Show resolved Hide resolved
settings Outdated
Comment on lines 70 to 71
PASS_NAME_GPG="theforeman/releases/foreman/$FOREMAN_VERSION-gpg"
PASS_NAME_KEY="theforeman/releases/foreman/$FOREMAN_VERSION-key"
Copy link
Member

Choose a reason for hiding this comment

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

This overrides the settings, which means you can't override it anymore as we do:

releases/pulpcore/settings
2:PASS_NAME_GPG="theforeman/releases/pulpcore/$VERSION-gpg"
3:PASS_NAME_KEY="theforeman/releases/pulpcore/$VERSION-key"

releases/foreman-debian/settings
1:PASS_NAME_GPG="theforeman/releases/foreman/debian-$VERSION-gpg"
2:PASS_NAME_KEY="theforeman/releases/foreman/debian-$VERSION-key"

releases/candlepin/settings
1:PASS_NAME_GPG="theforeman/releases/candlepin/$VERSION-gpg"
2:PASS_NAME_KEY="theforeman/releases/candlepin/$VERSION-key"

It's also not like we can use $PROJECT like this:

Suggested change
PASS_NAME_GPG="theforeman/releases/foreman/$FOREMAN_VERSION-gpg"
PASS_NAME_KEY="theforeman/releases/foreman/$FOREMAN_VERSION-key"
PASS_NAME_GPG="theforeman/releases/$PROJECT/$VERSION-gpg"
PASS_NAME_KEY="theforeman/releases/$PROJECT/$VERSION-key"

What I'm leaning to is move these definitions into releases/foreman/settings (and not have any default) but I don't think we load that now when inheriting settings.

Alternatively you can use:

Suggested change
PASS_NAME_GPG="theforeman/releases/foreman/$FOREMAN_VERSION-gpg"
PASS_NAME_KEY="theforeman/releases/foreman/$FOREMAN_VERSION-key"
PASS_NAME_GPG="${PASS_NAME_GPG:-theforeman/releases/foreman/$FOREMAN_VERSION-gpg}"
PASS_NAME_KEY="${PASS_NAME_KEY:-theforeman/releases/foreman/$FOREMAN_VERSION-key}"

Copy link
Member Author

Choose a reason for hiding this comment

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

dang.

so the problem I was trying to fix is:
When signing Katello, $OSES ends up '' as it first loaded katello/4.14/settings, then foreman/3.12/settings and then overwrote OSES with the value in settings.
To fix that, I moved the "load inherited foreman stuff" logic into load_settings, but that that meant that FOREMAN_VERSION is not yet populated when the old location of PASS_NAME_* was used, so I moved that too.

Copy link
Member

Choose a reason for hiding this comment

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

I added a commit that I think addresses the concerns. It's not pretty, but I think it'll work.

The values were specific to Foreman, but we have other projects. The
code is now modified to ensure the variables are set before they're
used.

It now also loads the common Foreman settings file for projects that
inherit the Foreman settings. Previously it only loaded the version
specific settings.

Some care is taken in Foreman's common settings to only export values
when no inheritance is taking place.
elif [[ "$FOREMAN_VERSION" != "none" ]]; then
# load foreman settings, which will contain keys etc
. releases/foreman/settings
. "releases/foreman/${FOREMAN_VERSION}/settings"
Copy link
Member Author

@evgeni evgeni Sep 9, 2024

Choose a reason for hiding this comment

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

mhh, no, this overwrites OSes that have been set in client/(version/)?settings before. damn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants