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

Overhaul config names #182

Merged
merged 6 commits into from
Oct 6, 2023
Merged

Conversation

jhiemstrawisc
Copy link
Member

In issue #130 we discussed making config variables hierarchical, such that each variable describes which service it affects. One goal was to keep the hierarchies flat (<= 2 layers). This PR breaks out the config variables and assigns them to various parent services. It also adds many new viper config variables to docs/parameters.yaml so that users can figure out which components of Pelican they have control over.

@jhiemstrawisc jhiemstrawisc requested review from bbockelm and removed request for bbockelm October 4, 2023 18:40
@jhiemstrawisc
Copy link
Member Author

@bbockelm I think this is ready for review, even though tests aren't passing yet. It looks like the failed tests are fetching and testing against the head of the repo's main branch. Any idea what's going on there?

@bbockelm
Copy link
Collaborator

bbockelm commented Oct 5, 2023

@jhiemstrawisc - I think the git merge algorithm was creating some of the issues you were seeing... just because it merges cleanly doesn't mean it's actually creating working code!

I did a quick rebase, touched up some items, and I think it's now back on track.

Copy link
Collaborator

@bbockelm bbockelm left a comment

Choose a reason for hiding this comment

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

Overall approach is good; some requests for renames embedded.

cmd/resources/xrootd.cfg Outdated Show resolved Hide resolved
cmd/resources/xrootd.cfg Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/resources/defaults.yaml Outdated Show resolved Hide resolved
docs/parameters.yaml Show resolved Hide resolved
docs/parameters.yaml Outdated Show resolved Hide resolved
docs/parameters.yaml Outdated Show resolved Hide resolved
config/config.go Dismissed Show dismissed Hide dismissed
jhiemstrawisc and others added 5 commits October 6, 2023 08:40
In issue PelicanPlatform#130 we discussed making config variables hierarchical, such that each variable
describes which service it affects. One goal was to keep the hierarchies flat (<= 2 layers).
This PR breaks out the config variables and assigns them to various parent services. It also
adds many new viper config variables to `docs/parameters.yaml` so that users can figure out
which components of Pelican they have control over.
Without this, if there are changes between the base and new code,
the params module may be in an inconsistent state.
Copy link
Collaborator

@bbockelm bbockelm left a comment

Choose a reason for hiding this comment

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

Updates post code-review look good.

This is a sufficiently large-scale change I'm going to go ahead and merge: any smaller issues can be done with follow-ups.

@bbockelm bbockelm merged commit e8237ea into PelicanPlatform:main Oct 6, 2023
6 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.

3 participants