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 positional argument to env commands for project #6387

Merged
merged 9 commits into from
Jan 15, 2025

Conversation

grahamplata
Copy link
Contributor

@grahamplata grahamplata commented Jan 9, 2025

Description
Normalizing env command usage to how project is implemented.

rill env supports flag and positional arg

  • rill env show --org=demo --project=rill-openrtb-prog-ads
  • rill env show rill-openrtb-prog-ads --org=demo

--env flag allows for the export of .env output

# Example: rill env show rill-openrtb-prog-ads --org=demo --env > .env
rill env show rill-openrtb-prog-ads --org=demo --env
TEST_KEY=TEST_VALUE

Additionally the character row count was increased https://github.com/lensesio/tableprinter/blob/master/tableprinter.go#L83-L84

// if RowCharLimit > 0 && RowTextWrap == true then wrap the line otherwise replace the trailing with "..."

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Verified

This commit was signed with the committer’s verified signature.
grahamplata Graham Plata

Verified

This commit was signed with the committer’s verified signature.
grahamplata Graham Plata
@grahamplata grahamplata marked this pull request as ready for review January 10, 2025 17:46
@grahamplata grahamplata self-assigned this Jan 10, 2025
cli/cmd/env/set.go Outdated Show resolved Hide resolved

setCmd := &cobra.Command{
Use: "set <key> <value>",
Args: cobra.ExactArgs(2),
Use: "set [<project-name>] [--env=key=value]",
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Did you consider set [<project>] <key> <value>, similar to e.g. rill project describe (link)? Not necessarily saying that's better, just curious which approach you think is more idiomatic? (IMO neither mixed required/optional positional args nor required flags feel intuitive, but can't think of other alternatives.)
  2. If you go ahead with this approach, note there are some non-generated docs that need to be updated (search docs/ for rill env set)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I see what you mean, I think I will stub it out and try

Copy link
Contributor Author

@grahamplata grahamplata Jan 13, 2025

Choose a reason for hiding this comment

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

rill env set some-project key value
Updated project variables

rill env show some-project
  NAME       VALUE        ENVIRONMENT
 ---------- ------------ -------------
  key        value
  TEST_KEY   TEST_VALUE

or assumed in dir

rill env set key value
Updated project variables

rill env show some-project
  NAME       VALUE        ENVIRONMENT
 ---------- ------------ -------------
  key        value
  TEST_KEY   TEST_VALUE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normalized on 1.

Comment on lines 55 to 59
if cmd.Flags().Changed("env") {
printEnv(envVars)
} else {
ch.PrintData(envVars)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A few thoughts:

  1. Using --env for this maybe feels confusing because we use --env to specify variable values in a couple other commands
  2. I wonder if people generally would prefer the shell export format instead of a table? I know I would. We could just have that as the only format if you agree.
  3. The printEnv function does not include v.Environment in the output (it allows for environment-specific overrides of variable values). One idea is to group using comments in the output, like this:
foo=bar
hello=world

# development
foo=bazz

# production
foo=foo

Copy link
Contributor Author

@grahamplata grahamplata Jan 13, 2025

Choose a reason for hiding this comment

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

  1. Agreed I'll adjust that
  2. I had the exact same thought I figured the use case would be to pipe to a dotenv or vault
  3. I think that is reasonable ~ is it worth providing a filter on that? ~ specify env

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is reasonable ~ is it worth providing a filter on that? ~ specify env

It kind of already does with the rill env show --environment flag. I say "kind of" because if an environment is provided, it actually evaluates the variables for that environment, i.e. it will return non-environment specific variables overwritten by the variables for the requested environment.

Sounds like this isn't completely clear – if you have any ideas for help menu or docs improvements here, feel free to send a PR :)

Verified

This commit was signed with the committer’s verified signature.
grahamplata Graham Plata

Verified

This commit was signed with the committer’s verified signature.
grahamplata Graham Plata
cli/cmd/env/set.go Outdated Show resolved Hide resolved
cli/cmd/env/show.go Outdated Show resolved Hide resolved
cli/cmd/env/show.go Outdated Show resolved Hide resolved
grahamplata and others added 3 commits January 14, 2025 12:33

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Benjamin Egelund-Müller <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Benjamin Egelund-Müller <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
grahamplata Graham Plata
@begelundmuller begelundmuller merged commit 42c37d8 into main Jan 15, 2025
7 checks passed
@begelundmuller begelundmuller deleted the gplata/cli-env branch January 15, 2025 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants