Skip to content
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

Merged
merged 18 commits into from
May 27, 2021
Merged

Conversation

omarismail
Copy link
Contributor

@omarismail omarismail commented May 25, 2021

Description

This PR adds two data sources: Organizations and Organization.

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

  1. Run the tests.
  2. Create a terraform config and use the data source for tfe_organization and tfe_organizations and output the values.

Output from acceptance tests

$ TFE_TOKEN=<token> TFE_HOSTNAME=<hostname> TF_ACC=1  go test -v ./tfe/...

Documentation

@omarismail omarismail marked this pull request as ready for review May 25, 2021 17:59
@omarismail omarismail requested review from a team May 25, 2021 17:59
Copy link
Member

@nfagerlund nfagerlund left a 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.

README.md Outdated Show resolved Hide resolved
@omarismail omarismail requested review from a team May 26, 2021 12:31
tfe/data_source_organization.go Outdated Show resolved Hide resolved
tfe/data_source_organization.go Outdated Show resolved Hide resolved
tfe/data_source_organization.go Outdated Show resolved Hide resolved
tfe/data_source_organization.go Outdated Show resolved Hide resolved
tfe/data_source_organization.go Outdated Show resolved Hide resolved
Type: schema.TypeList,
Computed: true,
Elem: &schema.Schema{Type: schema.TypeString},
},
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Member

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 🤷‍♂️

Copy link
Contributor Author

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.

Copy link
Member

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!

tfe/data_source_organizations.go Outdated Show resolved Hide resolved
website/docs/d/organization.html.markdown Outdated Show resolved Hide resolved
Copy link
Member

@chrisarcand chrisarcand left a 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`.
Copy link
Member

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.
Copy link
Member

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.

Suggested change
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 omarismail merged commit 7c996f6 into master May 27, 2021
@omarismail omarismail deleted the omar/org-data-soruce branch May 27, 2021 19:01
@dennisroche
Copy link

dennisroche commented Aug 30, 2021

@omarismail @chrisarcand can a new release of tfe be cut to include this change?

Last release was 0.25.3 on 19 May 2021.

@chrisarcand
Copy link
Member

@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!

@dennisroche
Copy link

Thanks @chrisarcand. We're eagerly waiting for both this and the workspace tagging.

@dennisroche
Copy link

0.26.0 was released. thank-you. 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants