-
Notifications
You must be signed in to change notification settings - Fork 23
Add data source attribute for owner email address #31
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
Conversation
internal/provider/provider_test.go
Outdated
@@ -19,7 +19,10 @@ func TestProvider(t *testing.T) { | |||
} | |||
|
|||
func TestWorkspace(t *testing.T) { | |||
t.Parallel() | |||
t.Setenv("CODER_WORKSPACE_OWNER", "owner123") | |||
t.Setenv("CODER_WORKSPACE_OWNER_NAME", "Workspace Owner Jr.") |
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 kinda messed this up in the other issue, and I think it could be better to remove this for now. Since we don't have a display name, it could be easy to confuse _name
with username.
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.
Sure. I was thinking it would be convenient to have a separate variable, because in the future we'll probably be able to get a display name from e.g. an OIDC provider. But you're right that for now, it's potentially confusing.
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's a good idea to expose the attributes. Interesting
ownerEmail := os.Getenv("CODER_WORKSPACE_OWNER_EMAIL") | ||
_ = rd.Set("owner_email", ownerEmail) |
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.
The other attributes handled by this function all have a non-empty default value in case the expected environment var is missing. But I can't think of a good default for the email address, and in my testing, there didn't seem to be any problem with leaving it empty.
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.
Pending owner_name
is removed in favor of owner
, this is good to merge! We should cut a new release too so it can be consumed by your other PR in coder/coder.
* Add data source attributes for owner name and email address * Fix incorrectly commented-out block * Remove "owner_name" field
This PR adds a new attribute to the
coder_workspace
data source:owner_email
, which is populated from theCODER_WORKSPACE_OWNER_EMAIL
environment variable.