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/run groups to cli #3753

Merged
merged 25 commits into from
Apr 4, 2024
Merged

Add/run groups to cli #3753

merged 25 commits into from
Apr 4, 2024

Conversation

mathnogueira
Copy link
Contributor

This PR...

Changes

Fixes

Checklist

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

Loom video

Add your loom video here if your work can be visualized

Copy link

vercel bot commented Mar 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
tracetest ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 4, 2024 8:41pm

@xoscar
Copy link
Contributor

xoscar commented Apr 4, 2024

@xoscar xoscar marked this pull request as ready for review April 4, 2024 18:19
runCmd.Flags().StringVarP(&runParams.DefinitionFile, "file", "f", "", "path to the definition file")
runCmd.Flags().StringVar(&runParams.ID, "id", "", "id of the resource to run")
runCmd.Flags().StringSliceVarP(&runParams.DefinitionFiles, "file", "f", []string{}, "path to the definition file (can be defined multiple times)")
runCmd.Flags().StringVarP(&runParams.ID, "id", "", "", "id of the resource to run (can be defined multiple times)")
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought about having multiple IDs, but I reverted the changes and forgot to change here:

Suggested change
runCmd.Flags().StringVarP(&runParams.ID, "id", "", "", "id of the resource to run (can be defined multiple times)")
runCmd.Flags().StringVarP(&runParams.ID, "id", "", "", "id of the resource to run")

Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't mind, I'll fix this in the next PR, I want to have this merged 🥴

Comment on lines +88 to +89
// ExitCLI will exit the process, so this return is just to satisfy the compiler
return "", err
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember this comic every time we need to satisfy our Go compiler xD :
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Man the compiler is really strict haha

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this formatter is only used on Cloud features, does it make sense to move it to the cloud package?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Once we finish this feature, I'll create a cleanup PR with some changes, I believe we might not need the separation between cloud and OSS, even if someone wants to replicate the behavior, it is handled by the server side which wont be supported by the OSS server

@xoscar xoscar merged commit 4f6165b into main Apr 4, 2024
39 checks passed
@xoscar xoscar deleted the add/run-groups-to-cli branch April 4, 2024 21:19
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.

3 participants