-
Notifications
You must be signed in to change notification settings - Fork 157
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 Data Source: Organizations & Organization #320
Conversation
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 hope to come back and test this later if I have time, but in the meantime I'll just post the readme suggestion I had.
Type: schema.TypeList, | ||
Computed: true, | ||
Elem: &schema.Schema{Type: schema.TypeString}, | ||
}, |
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 wonder if this is necessary? For workspaces, names
is a required argument you must provide; here in organizations, you're just fetching all organizations accessible by the authenticated token.
Since ids
(which matches the equivalent workspaces one) includes both names and IDs, this just feels like duplication, no?
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 put names
here as well because we use the organization's name
as a primary identifier throughout the API, and I figured a common usage pattern is that users would query this data source and iterate through the names. That being said, one could also just iterate through ids
(IDs/Names map) and do an action using the name
.
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.
Hmmm, I was actually wondering the opposite! We use id
elsewhere in the provider to mean the organization's name, so it seems like it could be confusing to use id
to mean the organization's external id 😬 plus you can't really use an organization's external id for anything right now, though of course that may change in the future.
Possibly terrible idea: what if we left ids
out of this data source completely and had names
as the only attribute?
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 being said, one could also just iterate through ids (IDs/Names map) and do an action using the name.
Yeah that was my thought
The fact that we don't ever use external IDs for orgs is problematic, and I hope we make some enhancements to allow for either in the future and actually change things like the reference CJ linked, but I can also appreciate that it might be overoptimizing for now. I think names
as the only attribute is just fine, actually! Maybe we could do that and leave the ID map for when (because we probably will) we migrate r/tfe_organization.id
over 🤷♂️
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 fact that we don't ever use external IDs for orgs is problematic
I'm in favor keeping names
. The user is used to using the organization name to execute operations, so now seeing "ids" only (and not name
) will be confusing. But like CJ pointed, we set id
elsewhere as name where its not actually the ID (external_id) but in fact is a name...😢.
I'm actually in favor of keeping both. names
because the user is already used to using that as the primary "id". And keeping the ids
map, even though it may not be immediately used, is good because we can put the pieces in place for a future migration to start using external_id as the primary identifier instead of names.
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.
Good discussion, sounds good to me!
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.
Two bits to tweak and then this is ready to roll! Also needs a CHANGELOG entry.
* `owners_team_saml_role_id` - The name of the "owners" team. | ||
* `permissions` - Represents the organization permissions | ||
* `session_timeout_minutes` - Session timeout after inactivity. Defaults to `20160`. | ||
* `session_remember_minutes` - Session expiration. Defaults to `20160`. |
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 all needs updating yet!
|
||
## Argument Reference | ||
|
||
No arguments are required. This retrieves the names and IDs of all the organizations. |
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 hint here that it may not be all the organizations.
No arguments are required. This retrieves the names and IDs of all the organizations. | |
No arguments are required. This retrieves the names and IDs of all the organizations readable by the provided token. |
@omarismail @chrisarcand can a new release of Last release was |
@dennisroche Yes indeed! There's some work being done to support the new workspace tagging feature; once that's in, we'll go ahead and cut a 0.26.0. Thanks for your patience! |
Thanks @chrisarcand. We're eagerly waiting for both this and the workspace tagging. |
0.26.0 was released. thank-you. 🚀 |
Description
This PR adds two data sources:
Organizations
andOrganization
.Organizations
returns a list of names and a map of IDs/Names.Organization
returns all the fields for a given organization, and requires a 'name' to query it.Testing plan
data
source fortfe_organization
andtfe_organizations
andoutput
the values.Output from acceptance tests
Documentation