Add login_type to coder_workspace_owner data source #235 #287
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
johnstcn
left a comment
There was a problem hiding this comment.
That was quick! You'll also need to update the integration tests to expect the new field.
|
Done! |
provider/workspace_owner.go
Outdated
| if login_type := os.Getenv("CODER_WORKSPACE_OWNER_LOGIN_TYPE"); login_type != "" { | ||
| _ = rd.Set("login_type", login_type) | ||
| } else { | ||
| _ = rd.Set("login_type", "none") |
There was a problem hiding this comment.
@mtojek should we instead set login_type to an empty string if Coder does not set the required env?
There was a problem hiding this comment.
Perhaps at the very least, drop a diag.Warn?
There was a problem hiding this comment.
should we instead set login_type to an empty string
This seems reasonable and consistent with other ENV vars. I would start with empty string, we can always revisit it in the future and replace it with none.
There was a problem hiding this comment.
This seems reasonable and consistent with other ENV vars. I would start with empty string, we can always revisit it in the future and replace it with
none.
Great, what about the warn?
There was a problem hiding this comment.
Adding a diag.Warn if the CODER_WORKSPACE_OWNER_LOGIN_TYPE environment variable is not present or empty would be a good indication to the user that they have a potential mismatch of the Coder version and provider version.
There was a problem hiding this comment.
Sorry, but taking a look to the diag package there is not Warn method.
https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/v2/diag
There was a problem hiding this comment.
Sorry! I should have been more specific -- you can append a diag.Diagnostic with severity diag.Warning to the second return argument of type diag.Diagnostics.
There was a problem hiding this comment.
Done, this is the first time I have to deal with the diags, so please let me know if this solution is correct 🙏🏼 .
mtojek
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
provider/workspace_owner.go
Outdated
| if login_type := os.Getenv("CODER_WORKSPACE_OWNER_LOGIN_TYPE"); login_type != "" { | ||
| _ = rd.Set("login_type", login_type) | ||
| } else { | ||
| _ = rd.Set("login_type", "none") |
There was a problem hiding this comment.
should we instead set login_type to an empty string
This seems reasonable and consistent with other ENV vars. I would start with empty string, we can always revisit it in the future and replace it with none.
|
All fixed @johnstcn thanks for your time! |
Co-authored-by: Cian Johnston <public@cianjohnston.ie>
johnstcn
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
Note that this won't be useful until coder/coder is updated to set CODER_WORKSPACE_OWNER_LOGIN_TYPE. This will require a separate PR.
This PR add the login type the the
coder_workspace_ownerdata source (Issue #235).I took some decisions (like the default value to
none), but if you think this should be different please let me know.Thanks!