Init config refactor #2035
Init config refactor #2035
Conversation
turetske
force-pushed
the
init-config-refactor
branch
2 times, most recently
from
42bf05f to
f55f544
Compare
There was a problem hiding this comment.
With this change, the log file initialization code runs when InitServer is executed. However, some code is executed before InitServer is reached. Because of this, all logs generated before InitServer runs will be output to the console even if the log file is configured. This could be problematic if something goes wrong before InitServer is reached, as the log file would not contain the relevant information.
turetske
force-pushed
the
init-config-refactor
branch
from
f55f544 to
40118c7
Compare
-- Moved InitConfig into InitServer and InitClient -- Replaced calls of InitConfig with InitServer and InitClient where possible -- Removed calls to InitConfigDir where possible -- Adjusted the tests where needed due to the changes made
-- Mostly due to unset ConfigDir that should be a tempDir -- Adjusted some params and a few other tests as well
turetske
force-pushed
the
init-config-refactor
branch
from
40118c7 to
9e7b6d0
Compare
turetske
requested review from
Saartank and
jhiemstrawisc
and removed request for
Saartank
|
|
||
| err := config.InitClient() | ||
| require.NoError(t, err) | ||
| test_utils.InitClient(t, make(map[string]any)) |
There was a problem hiding this comment.
This might be a bit easier to read if you nil to the client initializer instead of make(map[string]any).
| // 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. |
There was a problem hiding this comment.
This comment is no longer relevant.
| // 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. |
| // This will be revisited in the future | ||
| os := runtime.GOOS | ||
| if os == "windows" { | ||
| log.Warningln("No home directory found for user -- will check for configuration yaml in C:/ProgramData/pelican") |
There was a problem hiding this comment.
I think Windows uses back slashes as filepath separators, so the path in this comment should probably be C:\ProgramData\pelican -- safer yet, use whatever gets spit out by the filepath.Join() you call in the next line.
| require.NoError(t, err) | ||
| }) | ||
|
|
||
| exportMap := item.(map[string]interface{}) |
| originRaw, exists := confMap["Origin"] | ||
|
|
||
| if exists { |
There was a problem hiding this comment.
This might be a bit cleaner as:
if originRaw, exists := confMap["Origin"]; exists {
// blah
}
since originRaw is not used outside the scope of this condition.
I think it might also be a fatal error if we don't pass this existence check -- that would indicate the test thinks it was provided origin configuration, but that configuration doesn't contain Origin:.
| originMap := originRaw.(map[string]interface{}) | ||
|
|
||
| // Override the test directory from the config file with our temp directory | ||
| if exportsRaw, ok := originMap["Exports"]; ok { |
There was a problem hiding this comment.
In the top-level if, you use exists instead of ok to determine whether the entry is in the map -- I think ok is more canonically "go", but I honestly think "exists" is easier to read. Can you pick one or the other and use in both cases?
| err = os.WriteFile(filepath.Join(originDir, "hello_world.txt"), []byte("Hello, World!"), os.FileMode(0644)) | ||
| require.NoError(t, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The majority of the code in the if exportsRaw, ok := originMap["Exports"]; ok { is duplicated in the subsequent else -- can you turn some of it into an inline helper function somewhere?
| err = config.InitServer(ctx, modules) | ||
| require.NoError(t, err) | ||
|
|
||
| // Read in any config we may have set | ||
| var originConf interface{} | ||
| if originConfig != "" { |
There was a problem hiding this comment.
This check shouldn't be needed anymore because a recent change added:
if originConfig == "" {
originConfig = fedTestDefaultConfig
}
to the start of the test
Moved InitConfig into InitServer and InitClient and adjusts the tests as needed.
Also made InitConfigDir platform agnostic.
Ideally, I would like to be able to make InitConfig private, but its use in the cmd/config_printer test makes that impossible.
I'm worried InitConfig will keep being used in tests until we figure out a solution for that if the author/reviewer don't catch it organically.