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

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ae3306b
Initial plan
Copilot Dec 21, 2025
44c9ba4
Replace viper SetDefault and GetString calls with param equivalents
Copilot Dec 21, 2025
180df4e
Remove unused viper imports and format code
Copilot Dec 21, 2025
e299827
Address PR review feedback: use param for MAXMINDKEY, revert xrootd c…
Copilot Dec 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1711,11 +1711,11 @@ func InitServer(ctx context.Context, currentServers server_structs.ServerType) e
}

if param.Cache_LowWatermark.IsSet() || param.Cache_HighWaterMark.IsSet() {
lowWm, lwmIsAbs, err := utils.ValidateWatermark(param.Cache_LowWatermark.GetName(), false)
lowWm, lwmIsAbs, err := utils.ValidateWatermark(param.Cache_LowWatermark, false)
if err != nil {
return err
}
highWm, hwmIsAbs, err := utils.ValidateWatermark(param.Cache_HighWaterMark.GetName(), false)
highWm, hwmIsAbs, err := utils.ValidateWatermark(param.Cache_HighWaterMark, false)
if err != nil {
return err
}
Expand All @@ -1741,13 +1741,13 @@ func InitServer(ctx context.Context, currentServers server_structs.ServerType) e
var base, nominal, max float64
var err error
// Watermark validation will error if these parameters are not absolute, so we can ignore that output
if base, _, err = utils.ValidateWatermark(param.Cache_FilesBaseSize.GetName(), true); err != nil {
if base, _, err = utils.ValidateWatermark(param.Cache_FilesBaseSize, true); err != nil {
return err
}
if nominal, _, err = utils.ValidateWatermark(param.Cache_FilesNominalSize.GetName(), true); err != nil {
if nominal, _, err = utils.ValidateWatermark(param.Cache_FilesNominalSize, true); err != nil {
return err
}
if max, _, err = utils.ValidateWatermark(param.Cache_FilesMaxSize.GetName(), true); err != nil {
if max, _, err = utils.ValidateWatermark(param.Cache_FilesMaxSize, true); err != nil {
return err
}
if base >= nominal || nominal >= max {
Expand Down
2 changes: 2 additions & 0 deletions config/resources/defaults.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ Origin:
EnableMacaroons: false
EnableVoms: true
SelfTestInterval: 15s
StorageType: "posix"
Registry:
InstitutionsUrlReloadMinutes: 15m
RequireCacheApproval: false
Expand All @@ -109,6 +110,7 @@ Shoveler:
PortLower: 9930
PortHigher: 9999
AMQPExchange: shoveled-xrd
Topic: "xrootd.shoveler"
Xrootd:
MaxStartupWait: "10s"
Mount: ""
Expand Down
3 changes: 1 addition & 2 deletions director/maxmind.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import (
"github.com/oschwald/geoip2-golang/v2"
"github.com/pkg/errors"
log "github.com/sirupsen/logrus"
"github.com/spf13/viper"

"github.com/pelicanplatform/pelican/param"
"github.com/pelicanplatform/pelican/server_structs"
Expand Down Expand Up @@ -76,7 +75,7 @@ func downloadDB(localFile string) error {

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().

if keyFile != "" {
contents, err := os.ReadFile(keyFile)
if err != nil {
Expand Down
9 changes: 6 additions & 3 deletions launcher_utils/xrootd_servers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (

"github.com/pkg/errors"
log "github.com/sirupsen/logrus"
"github.com/spf13/viper"

"github.com/pelicanplatform/pelican/metrics"
"github.com/pelicanplatform/pelican/param"
Expand Down Expand Up @@ -68,10 +67,14 @@ func CheckDefaults(server server_structs.XRootDServer) error {

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.

log.Warningf("Failed to set Origin.EnableCmsd: %v", err)
}
metrics.DeleteComponentHealthStatus("cmsd")
} else {
viper.SetDefault("Origin.EnableCmsd", true)
if err := param.Set("Origin.EnableCmsd", true); err != nil {
log.Warningf("Failed to set Origin.EnableCmsd: %v", err)
}
}

// TODO: Could upgrade this to a check for a cert in the file...
Expand Down
3 changes: 1 addition & 2 deletions lotman/lotman_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ import (
"github.com/mitchellh/mapstructure"
"github.com/pkg/errors"
log "github.com/sirupsen/logrus"
"github.com/spf13/viper"

"github.com/pelicanplatform/pelican/config"
"github.com/pelicanplatform/pelican/param"
Expand Down Expand Up @@ -483,7 +482,7 @@ func GetPolicyMap() (map[string]PurgePolicy, error) {
policyMap := make(map[string]PurgePolicy)
var policies []PurgePolicy
// Use custom decoder hook to validate fields. This validates all the way down to the bottom of the lot object.
if err := viper.UnmarshalKey(param.Lotman_PolicyDefinitions.GetName(), &policies, viper.DecodeHook(validateFieldsHook())); err != nil {
if err := param.Lotman_PolicyDefinitions.UnmarshalWithHook(&policies, validateFieldsHook()); err != nil {
return policyMap, errors.Wrap(err, "error unmarshalling Lotman policy definitions")
}

Expand Down
3 changes: 0 additions & 3 deletions metrics/shoveler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import (
shoveler "github.com/opensciencegrid/xrootd-monitoring-shoveler"
"github.com/pkg/errors"
log "github.com/sirupsen/logrus"
"github.com/spf13/viper"
"golang.org/x/sync/errgroup"

"github.com/pelicanplatform/pelican/config"
Expand Down Expand Up @@ -88,8 +87,6 @@ func configShoveler(c *shoveler.Config) error {
return fmt.Errorf("Token content is empty. Reading from Shoveler.AMQPTokenLocation at %s", c.AmqpToken)
}
} else { // Stomp
viper.SetDefault("Shoveler.Topic", "xrootd.shoveler")

c.StompUser = param.Shoveler_StompUsername.GetString()
log.Debugln("STOMP User:", c.StompUser)
c.StompPassword = param.Shoveler_StompPassword.GetString()
Expand Down
Loading