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 ability to set envvars from the cli #179

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

adoublef
Copy link

I created a simple subcommand for setting environment variables from the CLI tool. There are a fair different ways that variables can be added but I've chosen one for now. If setting variables from a file would be of interest or setting via a prompt I am happy to alter

@CLAassistant
Copy link

CLAassistant commented Oct 17, 2023

CLA assistant check
All committers have signed the CLA.

@adoublef
Copy link
Author

Came across an issue #138 that also adds the ability to load from a file. I think this could be added with this PR potentially.

Copy link
Member

@arnauorriols arnauorriols left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution! This looks fairly good. Just a couple of adjustments and it will be ready to land.

deployctl.ts Show resolved Hide resolved
src/subcommands/env.ts Show resolved Hide resolved
}

const opts = {
envVars: await parsePairs(rawArgs._).catch((e) => error(e)),
Copy link
Member

Choose a reason for hiding this comment

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

We will want to introduce other operations related to envs: list, delete, export, import from .env file, etc. I think it would be preferable if we already plan for this by having a set subcommand (ie deployctl env set key=value key2=value2)

Copy link
Author

Choose a reason for hiding this comment

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

Ah good to know, I was unsure at first due to the only API I could see was to set (i.e making a PATCH request to the api) but this makes sense.

also with #185 looking to also add option for file would it make sense for us to use the same subcommand? some thing like env set [key=value] and env set --env-file? or better think about merging those later?

Copy link
Member

Choose a reason for hiding this comment

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

I was unsure at first due to the only API I could see was to set (i.e making a PATCH request to the api)

The PATCH is also used to remove envs, by setting null as value.

also with #185 looking to also add option for file would it make sense for us to use the same subcommand? some thing like env set [key=value] and env set --env-file? or better think about merging those later?

Yes, we definitely want that feature, and I think it makes sense to have it within the env subcommand. However, I wonder if we wouldn't prefer a separate env subcommand, like deployctl env set key=value [keyn=valuen] and deployctl env import [--env-file env-file]. In any case, let's coordinate with @drollinger.

Adding also @kwhinnery to the loop.

Copy link
Author

Choose a reason for hiding this comment

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

I would be happy with this, will check to see if @kwhinnery has any thoughts before implementing my changes as I have time to try and complete this soon

Copy link
Member

Choose a reason for hiding this comment

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

@adoublef I would go ahead and implement deployctl env set key=value and deployctl env import [--env-file env-file].

src/utils/pairs.ts Show resolved Hide resolved
src/subcommands/env.ts Show resolved Hide resolved
@adoublef
Copy link
Author

Have been away from this for a while, wondering if this is still something I can continue or shall close.

thanks

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