Skip to content
This repository was archived by the owner on Jan 15, 2024. It is now read-only.

Return team id when adding team #6

Merged
merged 4 commits into from
Oct 12, 2020
Merged

Conversation

jonathan-dorsey
Copy link
Contributor

@jonathan-dorsey jonathan-dorsey commented Oct 12, 2020

This pull request updates the Team code to return the computed id value for newly created teams. This is necessary in support of adding team resource functionality in the terraform provider. See grafana/terraform-provider-grafana#120

Also updated the test to use and validate the newly returned id value. While there, modified the Team code to use a value instead of a pointer. This is more consistent with some of the similar code in org, alert, user...

NOTE: A better approach to these methods may be to accept an instance of Team that contains all the values for creating a new team object. Then a fully populated Team instance could be returned. This would work around the need to continually add parameters (and maybe return values for computed fields) as fields are added to the Team object in Grafana. Though I think this is likely a better long-term approach, I instead followed the patterns set out in other resources for consistency sake.

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, but spotted nits.

@aknuds1 aknuds1 requested a review from marefr October 12, 2020 13:03
@jonathan-dorsey
Copy link
Contributor Author

@aknuds1 - thanks for the feedback. Suggestions incorporated. It's worth noting that most of the things you mentioned were done that way just for consistency sake with the org. I used that file as a template for the changes. We may want to enter a new issue to clean up the other files so the coding style is consistent everywhere in the new fork. Thanks again!

@aknuds1
Copy link
Contributor

aknuds1 commented Oct 12, 2020

@jonathan-dorsey Gotcha, I think it would be best to enable style linting, but that requires more work. I suggest we do that through a dedicated PR.

@aknuds1
Copy link
Contributor

aknuds1 commented Oct 12, 2020

I think @marefr wants to have a look, but will be busy until the end of the week.

@aknuds1 aknuds1 requested review from papagian and wbrowne October 12, 2020 13:52
@aknuds1
Copy link
Contributor

aknuds1 commented Oct 12, 2020

Actually no, this is good to go.

@aknuds1 aknuds1 merged commit c87fc20 into grafana:master Oct 12, 2020
@aknuds1
Copy link
Contributor

aknuds1 commented Oct 12, 2020

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants