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

feat(cli): Token Support #3255

Merged
merged 4 commits into from Oct 13, 2023
Merged

feat(cli): Token Support #3255

merged 4 commits into from Oct 13, 2023

Conversation

xoscar
Copy link
Collaborator

@xoscar xoscar commented Oct 12, 2023

This PR adds token support for the tracetest start command

Changes

  • Adds new env variable to support the TOKEN
  • Adds config to replace jwt and token from the given flags

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

Loom video

https://www.loom.com/share/a1405279a47a429cae4fe0bb8df25233

@xoscar xoscar self-assigned this Oct 12, 2023
@xoscar xoscar marked this pull request as ready for review October 12, 2023 22:30
@@ -49,6 +52,7 @@ func init() {
startCmd.Flags().StringVarP(&saveParams.organizationID, "organization", "", "", "organization id")
startCmd.Flags().StringVarP(&saveParams.environmentID, "environment", "", "", "environment id")
startCmd.Flags().StringVarP(&saveParams.agentApiKey, "api-key", "", "", "agent api key")
startCmd.Flags().StringVarP(&saveParams.tokenApiKey, "token", "", defaultToken, "token api key")
Copy link
Contributor

Choose a reason for hiding this comment

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

cli-api-keyinstead of token?

Copy link
Member

Choose a reason for hiding this comment

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

I'm kind of late to this discussion. But I think that cli-api-key is too close to api-key and it's hard to understand what's the difference between them.

I would suggest using either token or a more meaningful name for this flag (which I have no suggestions yet). The idea behind using token is that the user will get the value from the Token tab, so will be more intuitive for the user to create a connection between the value and the flag they need to use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with this statement too, I think we can go with token as it matches the SaaS cloud, then in the future we can decide to change it

Copy link
Member

@mathnogueira mathnogueira left a comment

Choose a reason for hiding this comment

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

Left a comment on the flag name discussion. It's not a blocker

@xoscar xoscar merged commit 0aeb269 into main Oct 13, 2023
36 of 38 checks passed
@xoscar xoscar deleted the feat/token-support branch October 13, 2023 16:52
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.

None yet

3 participants