- Notifications
You must be signed in to change notification settings - Fork22
feat: addcoder_env#174
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| return &schema.Resource{ | ||
| Description: "Use this resource to set an environment variable in a workspace.", | ||
| CreateContext: func(_ context.Context, rd *schema.ResourceData, _ interface{}) diag.Diagnostics { | ||
| rd.SetId(uuid.NewString()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I considered disallowing envs here, likePATH,CODER_AGENT_TOKEN, etc. However, I opted for doing this agent side instead (we'll log an error).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I left a few ideas for further improvement. What you proposed is good for MVP.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -80,6 +80,7 @@ func New() *schema.Provider { | |||
| "coder_app": appResource(), | |||
| "coder_metadata": metadataResource(), | |||
| "coder_script": scriptResource(), | |||
| "coder_env": envResource(), | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thinking loud: Do we need an "opposite" resource?coder_no_env to remove the ENV variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For sure, I'd say that goes as a prop:resource "coder_env" "" { name = "FOO", unset = true }. Discussion ongoing incoder/coder#10166.
| ForceNew: true, | ||
| Required: true, | ||
| }, | ||
| "value": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thinking loud:default_value if not set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What's the use-case? Maybe we can add it later if we identify one? Since this is an optional string the default value is empty string, which is just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Something similar to: using generic dot files if the user didn't provide a custom URL. But I agree to postpone it for later until we meet scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe that can be handled in terraform? Likevalue = something_something ? other : "fallback", I think this is OK since we're not really looking for user input for variables and it's unlikely you define acoder_env unless you have an idea of what it should be. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is fine for now, thanks for challenging
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Great work. <3 I am eager to test this :)
Uh oh!
There was an error while loading.Please reload this page.
I opted for the single-var approach (vs multi as described in#170) because I don't see a use-case for settinga lot of environment variables, and I feel it better matches the resource name and I foresee use-cases for per-environment settings (overwrite, unset, prepend, append, etc.).
Fixes#170
Refscoder/coder#10166