feat: Config flag error ignored during login #251 #259

qcserestipy · 2024-11-15T19:46:01Z

Fixes #251

This pull request is currently work in progress.

Description
This pull request focuses on configuration handling and the login process.

Changes

config flag is now properly handled in the runLogin function. Originally the default path was always taken.
Added ConfigPath to loginview
Added ValidateConfigPath function to ensure the configuration file path ends with .yaml or .yml.
Existing config files only get updated if credentials do not yet exist
Overwrite config entry when password or more attributes have changed for same name

Open Changes

Adding support for environment variable of config path

Even though this PR is still WIP I would be happy to receive your feedback on any code changes.

bupd

Thanks for your contribution.

Also we can store user-defined config path in data file. to persist across sessions.

update config default location to $XDG_CONFIG_HOME/harbor-cli/config.yaml
store the data file in $XDG_DATA_HOME/harbor-cli/data which is also ~/.local/share/harbor-cli/data. In the data file we can store the user defined config path.
With this we will be having the robust config option for the harbor-cli.

bupd · 2024-11-15T20:55:11Z

pkg/views/login/create.go

+			huh.NewInput().
+				Title("Config File").
+				Value(&loginView.Config).
+				Description("Path to the config file").
+				Validate(func(str string) error {
+					if strings.TrimSpace(str) == "" {
+						return errors.New("config file cannot be empty or only spaces")
+					}
+					if isValid := utils.ValidateConfigPath(str); !isValid {
+						return errors.New("The config file path is not valid. It should be a valid path with a .yaml or .yml extension")
+					}
+					return nil
+				}),


Instead of having config as an input to login command. We can have --config as global flag. with which user can specify the default config location to store the config.

bupd · 2024-11-15T21:02:39Z

@qcserestipy Please sign your commits

qcserestipy · 2024-11-15T22:42:39Z

@bupd Thank you for your feedback! Yes, you were right. The config was set as a global flag already. I removed the option from the login screen. The credential handling is now moved to the runLogin function only. I will start looking into the $XDG_CONFIG_HOME config handling you suggested.

qcserestipy · 2024-11-18T15:15:47Z

The changes ensure that the application consistently references the active configuration, manages multiple credential sets efficiently, and maintains a reliable state across different operations.

Key Changes

PersistentPreRun Function for Configuration Initialization
- Purpose: Ensures that the configuration is initialized before any command execution.
- Benefits: Guarantees that all commands have access to the necessary configuration data, preventing runtime errors related to uninitialized configurations.
Data File Logic for Tracking Active Configuration
- Purpose: Implements a data file (data.yaml) that keeps track of the last active configuration file (config.yaml).
- Benefits: Allows users to switch between multiple configuration files seamlessly without manually specifying the config path each time.
Global Pointers for DataFile and ConfigFile
- Purpose: Makes HarborData and HarborConfig accessible globally through pointers.
- Benefits: Simplifies access to configuration and data throughout the application, reducing the need for passing configuration objects between functions and packages.
Harbor Client for Credential Retrieval
- Purpose: Adjusts the Harbor client to retrieve credentials using the global configuration pointer.
- Benefits: Ensures that all client operations consistently use the currently active credentials, enhancing security and reliability.
Credential Management Enhancements in Login Command
- Purpose: Adds functionality to the login process to update credentials in the configuration file after a successful login.
- Benefits: Automatically manages and updates credential sets, reducing manual intervention and minimizing the risk of credential mismatches.

Showcase

1

The datafile is created and it contains info about the config path which is in the config2.yaml file

2 and 3

I log in to the server and choose a new config file called config.yaml. The login proceeds, the file is created and the data file is updated.

4

The data file contains information about an old file config3.yaml that does not exist anymore. The cli crashes. The user is informed to create a new blank config3.yaml or change the name to a new config file where the data file gets updated.

5

The usual harbor subcommands can read the peristent config file specified in data.yaml, or use a config file that is given on the fly by the --config flag.

qcserestipy · 2024-11-18T15:17:09Z

@bupd Is there a good way to test this code in unit tests?

Vad1mo · 2024-11-19T14:39:58Z

pkg/utils/config.go

+		log.Fatalf("Unable to determine user home directory: %v", err)
+	}
+
+	xdgConfigHome := os.Getenv("XDG_CONFIG_HOME")


does UserHomeDir() not already respect the XDG_CONFIG_HOME?

I followed the instructions in this specification. According to section 3 the xdg home should default to $HOME/.config in case the XDG environment variables are not set in the shell/env. Therefore I thought it could be helpful to check whether a user has a $HOME, since this might not be fulfilled for e.g. machine users.

But I will double check whether the result in the expected behavior, and want to include this in some unit testing.

Vad1mo · 2024-11-22T08:51:49Z

@qcserestipy make sure you comple the DCO part -> https://github.com/goharbor/harbor-cli/pull/259/checks?check_run_id=33351002182

…igpath flag check to determine if given Signed-off-by: qcserestipy <[email protected]>

Signed-off-by: qcserestipy <[email protected]>

…ion to runLogin function Signed-off-by: qcserestipy <[email protected]>

…e logic that keeps track of the last active config; Made DataFile and ConfigFile Available through global pointer. Adjusted harbor client to retrieve credentials through config pointer; Added funtionality to login run to update credentials after login Signed-off-by: qcserestipy <[email protected]>

Signed-off-by: qcserestipy <[email protected]>

…G flag Signed-off-by: qcserestipy <[email protected]>

…ionality for Config and Data Pointer Signed-off-by: qcserestipy <[email protected]>

…clean

Vad1mo · 2024-11-22T10:09:43Z

@qcserestipy can you also address the discussion points we can than move forward

…sts; added config tests; adapted login tests to work with new config management Signed-off-by: Patrick Eschenbach <[email protected]>

qcserestipy · 2024-11-22T15:00:31Z

@Vad1mo Thank you for making me aware of the sign-off issue, I tried to resolve with the link you mentioned. I added some tests for the config and adapted the tests for the login subcommand.

Are there other points that I did not cover, yet?

qcserestipy · 2024-11-22T15:02:34Z

I removed the WIP tag for now

…g pipeline about unused err in config_test.go Signed-off-by: Patrick Eschenbach <[email protected]>

bupd

LGTM! 🚀

bupd suggested changes Nov 15, 2024

View reviewed changes

Vad1mo reviewed Nov 19, 2024

View reviewed changes

qcserestipy added 7 commits November 22, 2024 10:28

Added configpath to loginview; Added configpath validator; Added conf…

89ad625

…igpath flag check to determine if given Signed-off-by: qcserestipy <[email protected]>

Added update credentials function

34a610e

Signed-off-by: qcserestipy <[email protected]>

Removed config file from login view; moved credential update or creat…

e7eb6bf

…ion to runLogin function Signed-off-by: qcserestipy <[email protected]>

Enhanced logging for login subcommand

e1f8692

Signed-off-by: qcserestipy <[email protected]>

Improved handling of initConfig function; introduced HARBOR_CLI_CONFI…

f7189b4

…G flag Signed-off-by: qcserestipy <[email protected]>

Improved Data and Config Pointer error handling; Included Mutex funct…

17a1886

…ionality for Config and Data Pointer Signed-off-by: qcserestipy <[email protected]>

qcserestipy force-pushed the 251_feat_config_flag_clean branch from 2b0ee51 to 17a1886 Compare November 22, 2024 09:29

Merge remote-tracking branch 'origin/main' into 251_feat_config_flag_…

96afdca

…clean

Vad1mo requested a review from bupd November 22, 2024 10:09

qcserestipy added 2 commits November 22, 2024 15:56

Added custom sync.Once to handle concurrent config environments in te…

28126e5

…sts; added config tests; adapted login tests to work with new config management Signed-off-by: Patrick Eschenbach <[email protected]>

Added custom sync.Once to handle concurrent config environments in te…

66716a8

…sts; added config tests; adapted login tests to work with new config management Signed-off-by: Patrick Eschenbach <[email protected]>

qcserestipy changed the title ~~!WIP; feat: Config flag error ignored during login #251 #258~~ feat: Config flag error ignored during login #251 #258 Nov 22, 2024

qcserestipy changed the title ~~feat: Config flag error ignored during login #251 #258~~ feat: Config flag error ignored during login #251 Nov 22, 2024

Vad1mo approved these changes Nov 22, 2024

View reviewed changes

Added latest changes from main; added changes suggested from reviewdo…

d700e14

…g pipeline about unused err in config_test.go Signed-off-by: Patrick Eschenbach <[email protected]>

bupd approved these changes Nov 26, 2024

View reviewed changes

Vad1mo merged commit 0d1f474 into goharbor:main Nov 26, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Config flag error ignored during login #251 #259

feat: Config flag error ignored during login #251 #259

qcserestipy commented Nov 15, 2024 •

edited

Loading

bupd left a comment

bupd Nov 15, 2024

bupd commented Nov 15, 2024

qcserestipy commented Nov 15, 2024

qcserestipy commented Nov 18, 2024

qcserestipy commented Nov 18, 2024 •

edited

Loading

Vad1mo Nov 19, 2024

qcserestipy Nov 19, 2024

qcserestipy Nov 19, 2024

Vad1mo commented Nov 22, 2024

Vad1mo commented Nov 22, 2024

qcserestipy commented Nov 22, 2024

qcserestipy commented Nov 22, 2024

bupd left a comment

feat: Config flag error ignored during login #251 #259

feat: Config flag error ignored during login #251 #259

Conversation

qcserestipy commented Nov 15, 2024 • edited Loading

bupd left a comment

Choose a reason for hiding this comment

bupd Nov 15, 2024

Choose a reason for hiding this comment

bupd commented Nov 15, 2024

qcserestipy commented Nov 15, 2024

qcserestipy commented Nov 18, 2024

Key Changes

Showcase

1

2 and 3

4

5

qcserestipy commented Nov 18, 2024 • edited Loading

Vad1mo Nov 19, 2024

Choose a reason for hiding this comment

qcserestipy Nov 19, 2024

Choose a reason for hiding this comment

qcserestipy Nov 19, 2024

Choose a reason for hiding this comment

Vad1mo commented Nov 22, 2024

Vad1mo commented Nov 22, 2024

qcserestipy commented Nov 22, 2024

qcserestipy commented Nov 22, 2024

bupd left a comment

Choose a reason for hiding this comment

qcserestipy commented Nov 15, 2024 •

edited

Loading

qcserestipy commented Nov 18, 2024 •

edited

Loading