Adds check to ensure config file exists. Fixes: #4513 #5387
+36
−20
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JAORMX
previously approved these changes
Jan 31, 2025
|
The test failure seems legit |
mesembria
force-pushed
the
4513-minder-should-validate-the-config-exists
branch
from
24d5e3d to
fbbeef1
Compare
evankanderson
previously approved these changes
Jan 31, 2025
cmd/cli/app/auth/auth_login.go
Outdated
Comment on lines
38
to
43
| // If config file is specified but doesn't exist, that's an error | ||
| if configFile := v.GetString("config"); configFile != "" { | ||
| if _, err := os.Stat(configFile); os.IsNotExist(err) { | ||
| return cli.MessageAndError("Config file does not exist", fmt.Errorf("file %s not found", configFile)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is this only on auth login and not on other commands (i.e. in ReadConfigFromViper)?
There was a problem hiding this comment.
Good idea. I'll move the logic up to that function and update the unit test as well.
|
Unit tests continue to fail during CI, but pass locally. I don't know enough to know what's different between the two envs. |
eleftherias
previously approved these changes
Feb 3, 2025
eleftherias
approved these changes
Feb 3, 2025
mesembria
force-pushed
the
4513-minder-should-validate-the-config-exists
branch
from
5a8458b to
c75ff4c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Added to check to ensure config file actually exists if flag is used.
Fixes #4513
Change Type
Mark the type of change your PR introduces:
Testing
No flag (minder auth login) - SAME AS PREVIOUSLY
Works as intended
Flag + no file (minder auth login --config) - SAME AS PREVIOUSLY
Usage
“Details: flag needs an argument: --config”
Flag + non-existant file (minder auth login --config blah) - FIXED
Message: Config file does not exist
Details: file blah not found
Flag + valid file (minder auth login --config config.yaml) - SAME AS PREVIOUSLY
Works as intended
Flag + invalid file(minder auth login --config go.sum) - SAME AS PREVIOUSLY
Error reading config file: While parsing config: yaml: unmarshal errors:
line 1: cannot unmarshal !!str
buf.bui...into map[string]interface {}Still took user to login page
Flag + invalid file (minder auth login --config LICENSE) - SAME AS PREVIOUSLY
yaml: line 92: mapping values are not allowed in this context
Review Checklist: