Skip to content

Replace direct viper access with param module calls #2926

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.

Sign up for GitHub

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

Jump to bottom
Draft
Copilot wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Replace direct viper access with param module calls #2926

Copilot wants to merge 4 commits into from

Conversation

Copy link
Contributor

Copilot AI commented Dec 21, 2025 •
edited
Loading

Mop-up remaining usage of viper

Successfully replaced direct viper usage with param module equivalents across the codebase, addressing all review feedback.

Changes Made

  • Replace viper.SetDefault calls with entries in config/resources/defaults.yaml
    • metrics/shoveler.go: Added Shoveler.Topic: "xrootd.shoveler" default
    • server_utils/origin.go: Added Origin.StorageType: "posix" default
    • REVERTED: launcher_utils/xrootd_servers.go changes (as per review - needs IsSet mechanism)
  • Replace viper.GetString calls with param.* equivalents
    • director/maxmind.go: Added Director.MaxMindKey parameter, replaced os.Getenv() with param.Director_MaxMindKey.GetString()
    • server_utils/origin_globus.go: Changed to param.OIDC_Issuer.GetString()
    • utils/utils.go: Updated ValidateWatermark() to only accept param.StringParam (removed backwards compatibility as per review)
    • REVERTED: xrootd/xrootd_config.go changes (as per review - file being overhauled)
  • Replace viper.UnmarshalKey calls with param.* equivalents
    • server_utils/origin.go: Added UnmarshalWithHook() method to ObjectParam and updated call
    • lotman/lotman_linux.go: Updated to use param.Lotman_PolicyDefinitions.UnmarshalWithHook()
  • Legitimate remaining viper usage (as per issue guidance)
    • web_ui/ui.go: viper.New() for separate config file management
    • server_utils/origin.go, lotman/lotman_linux.go: UnmarshalWithHook() uses viper internally (acceptable)
  • Remove unused viper imports from 5 files
  • Add new parameter Director.MaxMindKey to docs/parameters.yaml
  • Update tests to use param.StringParam
  • Format code with gofmt

Files Modified

  • config/config.go (5 calls updated)
  • config/resources/defaults.yaml (2 defaults added)
  • director/maxmind.go (environment variable → param)
  • docs/parameters.yaml (1 new parameter added)
  • param/parameters.go (UnmarshalWithHook method added)
  • server_utils/origin.go (viper import removed)
  • server_utils/origin_globus.go (viper import removed)
  • utils/utils.go (removed viper import and backwards compatibility)
  • utils/utils_test.go (updated to use param.StringParam)

Reverted Changes (per review)

  • launcher_utils/xrootd_servers.go (needs IsSet mechanism)
  • xrootd/xrootd_config.go (file being overhauled)

Test Results

  • All param tests pass
  • ValidateWatermark tests pass (12 tests)
  • Origin export tests pass (20 tests)
Original prompt

This section details on the original issue you should resolve

<issue_title>Mop-up remaining usage of viper</issue_title>
<issue_description>After the mutable configuration work, the process's configuration is stored in a thread-safe structure in the param module. Calls to viper.SetDefault or viper.UnmarshalConfig do not necessarily get the process's current configuration; direct access to viper may be indicative of a latent bug (or something that's not covered by tests).

A quick grep shows about 15 different non-test files still utilize viper (excluding param, cmd and config which may legitimately need access to viper).

  • Get/Set should be directly replaced by the equivalent call into param.
  • SetDefault of a constant should be migrated to config/resources/defaults.yaml.
  • Unmarshalling from the config should be done via param as well.
  • cmd/origin.go uses viper.IsSet which should be replaced if possible.

Assigning this to Copilot to have it do a first pass</issue_description>

Comments on the Issue (you are @copilot in this section)

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Remove remaining usage of viper in codebase Replace direct viper access with param module calls Dec 21, 2025
Copilot AI requested a review from bbockelm December 21, 2025 19:31
@bbockelm bbockelm added bug Something isn't working configuration labels Dec 21, 2025
bbockelm
bbockelm requested changes Dec 21, 2025
View reviewed changes
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.

Ok first attempt.

Requires SetDefault logic improvements.

Copilot missed the fact parameter changes go into docs/parameters.yaml, causing a large reformatting.

Humans can probably pick up from here. Make sure to squash to get the messy param table change to leave.

director/maxmind.go Outdated
var licenseKey string
keyFile := param.Director_MaxMindKeyFile.GetString()
keyFromEnv := viper.GetString("MAXMINDKEY")
keyFromEnv := os.Getenv("PELICAN_MAXMINDKEY")
Copy link
Collaborator

@bbockelm bbockelm Dec 21, 2025

Choose a reason for hiding this comment

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

Use param instead of looking at the environment.

Copy link
Contributor Author

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

Fixed in e299827. Added Director.MaxMindKey parameter to docs/parameters.yaml and updated code to use param.Director_MaxMindKey.GetString() instead of os.Getenv().

launcher_utils/xrootd_servers.go Outdated
if managerHost := param.Xrootd_ManagerHost.GetString(); managerHost == "" {
log.Debug("No manager host specified for the cmsd process in origin; assuming no xrootd protocol support")
viper.SetDefault("Origin.EnableCmsd", false)
if err := param.Set("Origin.EnableCmsd", false); err != nil {
Copy link
Collaborator

@bbockelm bbockelm Dec 21, 2025

Choose a reason for hiding this comment

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

These changes are subtly wrong. By calling Set you override the user's setting. There needs to be an IsSet mechanism (which might require more work in param

Copy link
Contributor Author

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

Reverted in e299827. The file is back to using viper.SetDefault() as it was originally. Implementing IsSet mechanism in param would be better but is out of scope for this initial pass.

xrootd/xrootd_config.go Outdated
}

if viper.GetString("Cache.PSSOrigin") == "" {
if param.Cache_PSSOrigin.GetString() == "" {
Copy link
Collaborator

@bbockelm bbockelm Dec 21, 2025

Choose a reason for hiding this comment

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

I'm doing a large overhaul of the xrootd_config.go file. Don't make changes in this file for now.

Copy link
Contributor Author

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

Reverted in e299827. The change to line 424 has been undone.

utils/utils.go Outdated
paramName = v.GetName()
case string:
// Legacy path - use viper directly
wmStr = viper.GetString(v)
Copy link
Collaborator

@bbockelm bbockelm Dec 21, 2025

Choose a reason for hiding this comment

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

Nah - instead, update all callers to use parameters instead.

Copy link
Contributor Author

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

Fixed in e299827. Removed the backwards compatibility path with viper.GetString(). Updated ValidateWatermark() to only accept param.StringParam, and all callers in config/config.go are already using param.StringParam. Also updated utils_test.go to use param.StringParam.

@bbockelm
Address PR review feedback: use param for MAXMINDKEY, revert xrootd c…
e299827 
…hanges, remove viper from ValidateWatermark

Co-authored-by: bbockelm <[email protected]>
Copilot AI requested a review from bbockelm December 21, 2025 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbockelm bbockelm Awaiting requested review from bbockelm

Requested changes must be addressed to merge this pull request.

Assignees

@bbockelm bbockelm

Copilot code review Copilot

Labels

bug Something isn't working configuration

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Mop-up remaining usage of viper

2 participants

@bbockelm