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 a couple of data sources #43

Merged
merged 2 commits into from
Jan 8, 2019
Merged

Add a couple of data sources #43

merged 2 commits into from
Jan 8, 2019

Conversation

svanharmelen
Copy link
Contributor

@svanharmelen svanharmelen commented Jan 3, 2019

  • tfe_ssh_key
  • tfe_team
  • tfe_team_access
  • tfe_workspace
  • tfe_workspace_ids

```hcl
data "tfe_workspace" "test" {
name = "my-workspace-name"
organization = "my-org-name"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm a little concerned about this interface being unwieldy, but I'm not sure whether the thing I want is possible or not...

My use case for this is shown in this example policies config. Basically I have a bunch of policy set resources, and each one needs to provide a bunch of workspace IDs, which I'd rather access by name. But with per-workspace data sources, I'd need to write four extra lines of configuration for every name I want to use, and I'd still have the problem of double-bookkeeping, where I have to keep my set of tfe_workspace data sources up to date. That's not too onerous in that toy example, but in a real-life situation with 50+ workspaces instead of just 5, it's kind of brutal.

Would it be possible to also make a tfe_workspaces or tfe_workspace_ids data source, which would have an ids attribute with a map value? Or is that not feasible because of something about the way providers work?

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 think a data source can have an ids attribute which contains a map of workspace names and their external IDs. But in that case I think the question is what kind of filter you want to use to select which IDs should be added to the map?

If that filter requires you to specify all names of the workspaces you want to select, you end up with having to declare the the same list twice. Once in the tfe_workspace_ids data source and once in the tfe_policy_set resource. Yet I agree that this would still be better then the current solution for your use case.

But after looking at your example, I think there is a much better way to make this resource user friendly. I don't think we should mix and match workspace IDs in TFE resources. So instead of using the external IDs, we should probably just use the workspace names here.

Exporting the external ID of a workspace is only done for specific use cases (see #19 for more details), so that ID should itself not be used in other resources. I guess this one slipped through when reviewing the PR.

So I'll work on a PR to update the tfe_policy_set resource today so you can have a look at it when you get online. I think that would be a much better solution that using a data source with a map.

Copy link
Contributor Author

@svanharmelen svanharmelen Jan 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please checkout PR #44 and then mainly the updated docs as those will show the UX changes when using the resource.

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 just added a new data source to this PR, called tfe_workspace_ids which I think solves your use case.

- tfe_ssh_key
- tfe_team
- tfe_team_access
- tfe_workspace
- tfe_workspace_ids
website/docs/d/workspace.html.markdown Outdated Show resolved Hide resolved
website/docs/d/workspace.html.markdown Outdated Show resolved Hide resolved
tfe/data_source_ssh_key.go Outdated Show resolved Hide resolved
tfe/data_source_ssh_key_test.go Show resolved Hide resolved
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few questions around the design of these resources but they otherwise LGTM 👍

tfe/data_source_ssh_key_test.go Show resolved Hide resolved
},

"vcs_repo": &schema.Schema{
Type: schema.TypeSet,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given this is a Data Source this'll need to be a List rather than a Set (I'd question if this information is likely to be used in a data source, however?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update it to a list 👍

Not sure if people need it, but since the info is available I think we should just return it. Pretty sure that if I remove it that within a few days or weeks someone comes to ask for it 😉

tfe/data_source_workspace_ids.go Show resolved Hide resolved
tfe/data_source_workspace_ids.go Show resolved Hide resolved
@svanharmelen svanharmelen dismissed bill-rich’s stale review January 8, 2019 12:52

I addressed the feedback and @tombuildstuff review afterwards

@svanharmelen svanharmelen merged commit 7fadd27 into master Jan 8, 2019
@svanharmelen svanharmelen deleted the svh/f-data-sources branch January 8, 2019 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants