Code cleanup in response to PR comments

Author: Emma Turetsky

client/fed_test.go

@@ -900,7 +900,7 @@ func TestObjectList405Error(t *testing.T) {
 // end-to-end
 func TestClientUnpack(t *testing.T) {
 	server_utils.ResetTestState()
-	test_utils.InitClient(t, make(map[string]any))
+	test_utils.InitClient(t, nil)
 
 	fed := fed_test_utils.NewFedTest(t, bothPublicOriginCfg)
 	export := fed.Exports[0]

cmd/plugin_test.go

@@ -589,8 +589,6 @@ func TestPluginDirectRead(t *testing.T) {
 	defer server_utils.ResetTestState()
 
 	dirName := t.TempDir()
-	// We are purposely creating a test with a config of a single-space.
-	// The empty string indicates using the default config, which we don't want.
 
 	fed := fed_test_utils.NewFedTest(t, publicTestOrigin)
 	host := param.Server_Hostname.GetString() + ":" + strconv.Itoa(param.Server_WebPort.GetInt())
@@ -873,8 +871,6 @@ func TestPluginRecursiveDownload(t *testing.T) {
 
 	dirName := t.TempDir()
 
-	// We are purposely creating a test with a config of a single-space.
-	// The empty string indicates using the default config, which we don't want.
 	fed := fed_test_utils.NewFedTest(t, publicTestOrigin)
 	host := param.Server_Hostname.GetString() + ":" + strconv.Itoa(param.Server_WebPort.GetInt())
 

config/config.go

@@ -511,8 +511,9 @@ func getConfigBase() string {
 	if err != nil {
 		os := runtime.GOOS
 		if os == "windows" {
-			log.Warningln("No home directory found for user -- will check for configuration yaml in C:/ProgramData/pelican")
-			return filepath.Join("C:", "ProgramData", "pelican")
+			windowsPath := filepath.Join("C:", "ProgramData", "pelican")
+			log.Warningln("No home directory found for user -- will check for configuration yaml in ", windowsPath)
+			return windowsPath
 		}
 		log.Warningln("No home directory found for user -- will check for configuration yaml in /etc/pelican/")
 		return filepath.Join("/etc", "pelican")

fed_test_utils/fed.go

@@ -176,79 +176,66 @@ func NewFedTest(t *testing.T, originConfig string) (ft *FedTest) {
 	require.NoError(t, err)
 
 	// Read in any config we may have set
-	var originConf interface{}
-	if originConfig != "" {
-		viper.SetConfigType("yaml")
-		err = viper.MergeConfig(strings.NewReader(originConfig))
-		require.NoError(t, err, "error reading config")
-
-		err := yaml.Unmarshal([]byte(originConfig), &originConf)
-		require.NoError(t, err, "error unmarshalling into interface")
-
-		confMap := originConf.(map[string]interface{})
-
-		originRaw, exists := confMap["Origin"]
-
-		if exists {
-			originMap := originRaw.(map[string]interface{})
-
-			// Override the test directory from the config file with our temp directory
-			if exportsRaw, ok := originMap["Exports"]; ok {
-				for i, item := range exportsRaw.([]interface{}) {
-					originDir, err := os.MkdirTemp("", fmt.Sprintf("Export%d", i))
-					assert.NoError(t, err)
-					t.Cleanup(func() {
-						err := os.RemoveAll(originDir)
-						require.NoError(t, err)
-					})
-
-					exportMap := item.(map[string]interface{})
-					exportMap["StoragePrefix"] = originDir
-
-					// Change the permissions of the temporary origin directory
-					permissions = os.FileMode(0755)
-					err = os.Chmod(originDir, permissions)
-					require.NoError(t, err)
+	var importedConf any
+	viper.SetConfigType("yaml")
+	err = viper.MergeConfig(strings.NewReader(originConfig))
+	require.NoError(t, err, "error reading config")
 
-					// Change ownership on the temporary origin directory so files can be uploaded
-					uinfo, err := config.GetDaemonUserInfo()
-					require.NoError(t, err)
-					require.NoError(t, os.Chown(originDir, uinfo.Uid, uinfo.Gid))
+	err = yaml.Unmarshal([]byte(originConfig), &importedConf)
+	require.NoError(t, err, "error unmarshalling into interface")
 
-					// Start off with a Hello World file we can use for testing in each of our exports
-					err = os.WriteFile(filepath.Join(originDir, "hello_world.txt"), []byte("Hello, World!"), os.FileMode(0644))
-					require.NoError(t, err)
-				}
-			} else {
-				originDir, err := os.MkdirTemp("", fmt.Sprintf("Export%s", "test"))
+	confMap := importedConf.(map[string]any)
+
+	if originRaw, exists := confMap["Origin"]; exists {
+		originMap := originRaw.(map[string]any)
+
+		overrideTemp := func(storageDir string, exportMap map[string]any) {
+			exportMap["StoragePrefix"] = storageDir
+
+			// Change the permissions of the temporary origin directory
+			permissions = os.FileMode(0755)
+			err = os.Chmod(storageDir, permissions)
+			require.NoError(t, err)
+
+			// Change ownership on the temporary origin directory so files can be uploaded
+			uinfo, err := config.GetDaemonUserInfo()
+			require.NoError(t, err)
+			require.NoError(t, os.Chown(storageDir, uinfo.Uid, uinfo.Gid))
+
+			// Start off with a Hello World file we can use for testing in each of our exports
+			err = os.WriteFile(filepath.Join(storageDir, "hello_world.txt"), []byte("Hello, World!"), os.FileMode(0644))
+			require.NoError(t, err)
+		}
+
+		// Override the test directory from the config file with our temp directory
+		if exportsRaw, exists := originMap["Exports"]; exists {
+			for i, item := range exportsRaw.([]any) {
+				originDir, err := os.MkdirTemp("", fmt.Sprintf("Export%d", i))
 				assert.NoError(t, err)
 				t.Cleanup(func() {
 					err := os.RemoveAll(originDir)
 					require.NoError(t, err)
 				})
 
-				originMap["StoragePrefix"] = originDir
-
-				permissions = os.FileMode(0755)
-				err = os.Chmod(originDir, permissions)
-				require.NoError(t, err)
-
-				// Change ownership on the temporary origin directory so files can be uploaded
-				uinfo, err := config.GetDaemonUserInfo()
+				exportMap := item.(map[string]any)
+				overrideTemp(originDir, exportMap)
+			}
+		} else {
+			originDir, err := os.MkdirTemp("", fmt.Sprintf("Export%s", "test"))
+			assert.NoError(t, err)
+			t.Cleanup(func() {
+				err := os.RemoveAll(originDir)
 				require.NoError(t, err)
-				require.NoError(t, os.Chown(originDir, uinfo.Uid, uinfo.Gid))
+			})
 
-				// Start off with a Hello World file we can use for testing in the StoragePrefix
-				err = os.WriteFile(filepath.Join(originDir, "hello_world.txt"), []byte("Hello, World!"), os.FileMode(0644))
-				require.NoError(t, err)
-			}
+			overrideTemp(originDir, originMap)
 		}
 	}
 
 	confDir := t.TempDir()
 	outputPath := filepath.Join(confDir, "tempfile_*.yaml")
 
-	outputData, err := yaml.Marshal(&originConf)
+	outputData, err := yaml.Marshal(&importedConf)
 	require.NoError(t, err, "error marshalling struct into yaml format")
 
 	err = os.WriteFile(outputPath, outputData, 0644)

lotman/lotman_test.go

@@ -148,7 +148,7 @@ func TestLotmanInit(t *testing.T) {
 
 	t.Run("TestGoodInit", func(t *testing.T) {
 		viper.Set("Log.Level", "debug")
-		viper.Set("Cache.DataLocations", []string{})
+		viper.Set(param.Cache_DataLocations.GetName(), []string{})
 		server := getMockDiscoveryHost()
 		// Set the Federation.DiscoveryUrl to the test server's URL
 		// Lotman uses the discovered URLs/keys to determine some aspects of lot ownership

xrootd/authorization_test.go

@@ -478,7 +478,7 @@ func TestEmitCfg(t *testing.T) {
 
 	defer server_utils.ResetTestState()
 
-	test_utils.InitClient(t, map[string]any{})
+	test_utils.InitClient(t, nil)
 	viper.Set(param.Origin_RunLocation.GetName(), dirname)
 
 	configTester := func(cfg *ScitokensCfg, configResult string) func(t *testing.T) {
@@ -557,7 +557,7 @@ func TestLoadScitokensConfig(t *testing.T) {
 
 	defer server_utils.ResetTestState()
 
-	test_utils.InitClient(t, map[string]any{})
+	test_utils.InitClient(t, nil)
 
 	viper.Set(param.Origin_RunLocation.GetName(), dirname)
 
@@ -595,6 +595,7 @@ func TestMergeConfig(t *testing.T) {
 	viper.Set(param.Origin_Port.GetName(), 8443)
 	viper.Set(param.Origin_StoragePrefix.GetName(), "/")
 	viper.Set(param.Origin_FederationPrefix.GetName(), "/")
+	viper.Set("ConfigDir", dirname)
 	// We don't inherit any defaults at this level of code -- in order to recognize
 	// that this is an authorized prefix, we must set either EnableReads && !EnablePublicReads
 	// or EnableWrites