- Notifications
You must be signed in to change notification settings - Fork22
Add login_type to coder_workspace_owner data source #235#287
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
github-actionsbot commentedSep 10, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@mtojek should we instead setlogin_type to an empty string if Coder does not set the required env?
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.
Perhaps at the very least, drop adiag.Warn?
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.
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 withnone.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Adding adiag.Warn if theCODER_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry, but taking a look to thediag package there is notWarn method.
https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/v2/diag
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.
Sorry! I should have been more specific -- you can append adiag.Diagnostic with severitydiag.Warning to the second return argument of typediag.Diagnostics.
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.
Done, this is the first time I have to deal with thediags, so please let me know if this solution is correct 🙏🏼 .
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 withnone.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
All fixed@johnstcn thanks for your time! |
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Cian Johnston <public@cianjohnston.ie>
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.
Thanks for your contribution!
Note that this won't be useful untilcoder/coder is updated to setCODER_WORKSPACE_OWNER_LOGIN_TYPE. This will require a separate PR.
2598aa7 intocoder:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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!