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

Adding team resource functionality #120

Merged
merged 11 commits into from
Oct 14, 2020

Conversation

jonathan-dorsey
Copy link
Contributor

This pull request adds team and team member functionality to the provider. Note: This code won't work until the PR nytm/go-grafana-api#64 is incorporated into the Grafana go api. We need the team id to be returned to the provider when a team is added.

@trotttrotttrott
Copy link
Member

Hi @jonathan-dorsey, this looks great. Since you opened this, the api client got switched to https://github.com/grafana/grafana-api-golang-client so we can have folks in the grafana org maintain it. Would you mind updating and rebasing?

@jonathan-dorsey
Copy link
Contributor Author

Hi @jonathan-dorsey, this looks great. Since you opened this, the api client got switched to https://github.com/grafana/grafana-api-golang-client so we can have folks in the grafana org maintain it. Would you mind updating and rebasing?

@trotttrotttrott - thanks for the update! I opened a new PR for the grafana project. Let me know if there is anything else you would like me to change/add.

@jonathan-dorsey
Copy link
Contributor Author

@trotttrotttrott - PR is ready for review. I updated the project to use the new version of the go api that includes my changes for the team ID.

Copy link
Member

@trotttrotttrott trotttrotttrott left a comment

Choose a reason for hiding this comment

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

Could you run go mod vendor so vendor/ is updated? Not sure why CI uses -mod=readonly. Will sort that out later. Also if you don't mind, go mod tidy. There's some extra API client references left over.

@jonathan-dorsey
Copy link
Contributor Author

@trotttrotttrott - commands run. Let me know if you need anything else. Thanks!

Copy link
Member

@trotttrotttrott trotttrotttrott left a comment

Choose a reason for hiding this comment

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

@jonathan-dorsey I found a few more things that need attention. Apologies for the back and forth.

grafana/resource_team.go Outdated Show resolved Hide resolved
grafana/resource_team.go Outdated Show resolved Hide resolved
grafana/resource_team.go Outdated Show resolved Hide resolved
grafana/resource_team.go Outdated Show resolved Hide resolved
grafana/resource_team.go Outdated Show resolved Hide resolved
website/docs/r/team.html.md Outdated Show resolved Hide resolved
@jonathan-dorsey
Copy link
Contributor Author

@trotttrotttrott - no worries! I appreciate the feedback and these are all good changes. A lot of these were just bad changes I furthered by copying from the org code. I am almost ready to start using the provider to manage a new Grafana instance. During that process I will do a lot more testing of edge scenarios and enter new defects for the org code as well. I would like to add some tests for some of those 404/409 errors, but I think it would require either applying changes to Grafana outside of terraform or manipulating the terraform state within the test and I'm not sure how to do either without some more research.

At any rate - changes have been incorporated and should be ready for another review. Thanks again!

Copy link
Member

@trotttrotttrott trotttrotttrott left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @jonathan-dorsey!

@trotttrotttrott trotttrotttrott merged commit d27a4f1 into grafana:master Oct 14, 2020
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.

2 participants