Skip to content
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.

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

add common flags that are shared between commands (user, token, config) #89

Merged
merged 2 commits into from
Jul 19, 2024

Conversation

consolethinks
Copy link
Collaborator

This commit moves two flags (user and token) and adds a (for now unused) config flag as persistent flags to the root command.

This means that the flags can be specified before or after the subcommand, and they don't have to be declared for each subcommand. The handling of them still happens in subcommands.

@sbliven
Copy link
Member

sbliven commented Jul 18, 2024

I tried testing and get a segfault:

$ /Users/bliven_s/git/scicat-cli/dist/scicat-cli_darwin_amd64_v1/scicat-cli  --token $(op read $SCICAT_TOKEN_QA) datasetIngestor --testenv metadata.json
2024/07/18 15:41:38 Latest version: 2.2.2
2024/07/18 15:41:38 Your version of this program is up-to-date
2024/07/18 15:41:39 You are about to add a dataset to the === test === data catalog environment...
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0x13603a6]

goroutine 1 [running]:
github.com/paulscherrerinstitute/scicat/datasetUtils.GetUserInfoFromToken(0xc00028c101?, {0x1448e03, 0x1e}, {0x205b873fa, 0x41})
	github.com/paulscherrerinstitute/scicat/datasetUtils/getUserInfoFromToken.go:22 +0xe6
github.com/paulscherrerinstitute/scicat/datasetUtils.(*RealAuthenticator).GetUserInfoFromToken(0xc000207470?, 0x1083ad8?, {0x1448e03?, 0xc0002074c0?}, {0x205b873fa?, 0x176cc80?})
	github.com/paulscherrerinstitute/scicat/datasetUtils/authenticate.go:70 +0x2c
github.com/paulscherrerinstitute/scicat/datasetUtils.Authenticate({0x14f5568, 0x17a83c0}, 0x143d3c8?, {0x1448e03, 0x1e}, 0xc0002078c0, 0xc0002078b0)
	github.com/paulscherrerinstitute/scicat/datasetUtils/authenticate.go:47 +0x34b
github.com/paulscherrerinstitute/scicat/cmd/commands.glob..func4(0xc00020eb00?, {0xc000130840, 0x1, 0x143ce20?})
	github.com/paulscherrerinstitute/scicat/cmd/commands/datasetIngestor.go:169 +0xd2c
github.com/spf13/cobra.(*Command).execute(0x1773780, {0xc000130800, 0x4, 0x4})
	github.com/spf13/[email protected]/command.go:987 +0xaa3
github.com/spf13/cobra.(*Command).ExecuteC(0x1774300)
	github.com/spf13/[email protected]/command.go:1115 +0x3ff
github.com/spf13/cobra.(*Command).Execute(...)
	github.com/spf13/[email protected]/command.go:1039
github.com/paulscherrerinstitute/scicat/cmd/commands.Execute()
	github.com/paulscherrerinstitute/scicat/cmd/commands/root.go:20 +0x1a
main.main()
	github.com/paulscherrerinstitute/scicat/cmd/main.go:6 +0xf

@sbliven
Copy link
Member

sbliven commented Jul 18, 2024

The error is from an incorrect token (I had a trailing newline accidently). It should be fixed but it's not related to this PR.

Copy link
Member

@sbliven sbliven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the approach of making authentication options global. I also like how cobra handles them; for instance you can put --token either before or after the datasetIngestor subcommand.

I think more options should also be made global: --version; environment selection (--testenv/--devenv, which could be combined into --env=[dev|qa|test|production]); --noninteractive possibly. If these need more discussion they can be moved to a separate issue.

@consolethinks
Copy link
Collaborator Author

consolethinks commented Jul 19, 2024

I like the approach of making authentication options global. I also like how cobra handles them; for instance you can put --token either before or after the datasetIngestor subcommand.

I think more options should also be made global: --version; environment selection (--testenv/--devenv, which could be combined into --env=[dev|qa|test|production]); --noninteractive possibly. If these need more discussion they can be moved to a separate issue.

the environment switches are left in for now, but eventually I would like to replace them with some kind of config file. I'm generally not a fan of hard coded endpoints. However, the version idea is a good point and will add that to this PR.

Copy link
Member

@sbliven sbliven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's leave the env for now. Can you make an issue for it though so we don't forget?

@consolethinks consolethinks merged commit 5e4f554 into main Jul 19, 2024
3 checks passed
@consolethinks consolethinks deleted the feature/common_flags branch July 19, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants