-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat: Add boundary_account
data source
#493
Conversation
f86a01e
to
d85ee4c
Compare
} | ||
|
||
var accountId string | ||
for _, account := range accounts { |
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.
If we have the line above that always checks to make sure we only have 1 account (if len(accounts) > 1
), do we still need the for loop? Won't there always be only 1 account in the list at this point?
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 point. What do you suggest here?
We could just get rid of that block and go directly to
arr, err := acl.Read(ctx, accounts[0].Id)
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.
For Boundary List calls where we can have multiple items, I wonder if we should also always return 1 time for datasources or return a list which could have either 1 item in it or more than 1 item? Right now we follow a pattern of always returning 1 item for a data source.
Would customers come back to ask for a new datasource that can return all items of a resource based on attributes/filters passed in because they do not want to repeat calls to a datasource to get more data.
I'll leave it to Louis to share his thoughts on this.
cc: @louisruch
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 question. I had similar thoughts. Right now, the current data sources just do a search by name
. Is that all we want here? Do we want to filter by other attributes?
Alternatively, we could just start with this current approach for now and iterate in the future.
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.
As far as I am aware a datasource must always match to a single resource Boundary. This is needed so you can bring in a resource by some filter and then use the id or other values in other resources. And this will be done via tf mapping.
As for future filters I think we definitely will want to use more than just name, but its a great place to start and we can expand as needed.
d85ee4c
to
48bd178
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.
Minor comment. Otherwise looks good. Thanks 🙏
48bd178
to
51dcbd6
Compare
This PR adds a
boundary_account
data sourceTest