-
Notifications
You must be signed in to change notification settings - Fork 159
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
Conversation
svanharmelen
commented
Jan 3, 2019
•
edited
Loading
edited
- tfe_ssh_key
- tfe_team
- tfe_team_access
- tfe_workspace
- tfe_workspace_ids
83ac602
to
fbec9cd
Compare
```hcl | ||
data "tfe_workspace" "test" { | ||
name = "my-workspace-name" | ||
organization = "my-org-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.
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?
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 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.
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.
Please checkout PR #44 and then mainly the updated docs as those will show the UX changes when using the resource.
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 just added a new data source to this PR, called tfe_workspace_ids
which I think solves your use case.
fbec9cd
to
c5c8bdc
Compare
- tfe_ssh_key - tfe_team - tfe_team_access - tfe_workspace - tfe_workspace_ids
c5c8bdc
to
652d814
Compare
908ebc0
to
5074253
Compare
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.
a few questions around the design of these resources but they otherwise LGTM 👍
tfe/data_source_workspace.go
Outdated
}, | ||
|
||
"vcs_repo": &schema.Schema{ | ||
Type: schema.TypeSet, |
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.
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?)
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.
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 😉
5074253
to
4b05e35
Compare
I addressed the feedback and @tombuildstuff review afterwards