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 team API calls #136

Merged
merged 6 commits into from
Aug 30, 2021
Merged

Add team API calls #136

merged 6 commits into from
Aug 30, 2021

Conversation

RLRabinowitz
Copy link
Contributor

@RLRabinowitz RLRabinowitz commented Aug 30, 2021

Issue & Steps to Reproduce / Feature Request

Feature request: Support Teams in the provider.

For starters, we need to implement the team API calls relevant for the team resource and data

Solution

Implement get/create/update/delete/get-all API calls

@RLRabinowitz RLRabinowitz self-assigned this Aug 30, 2021
@RLRabinowitz RLRabinowitz requested a review from a team August 30, 2021 07:27
Comment on lines 1 to 4
provider "env0" {
api_key = var.env0_api_key # or use ENV0_API_KEY
api_secret = var.env0_api_secret # or use ENV0_API_SECRET
api_key = var.env0_api_key # or use ENV0_API_KEY
api_secret = var.env0_api_secret # or use ENV0_API_SECRET
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

go fmt

@@ -10,5 +10,5 @@ data "env0_project" "project" {

resource "env0_cloud_credentials_project_assignment" "example" {
credential_id = env0_aws_credentials.credentials.id
project_id = data.env0_project.project.id
project_id = data.env0_project.project.id
Copy link
Contributor Author

Choose a reason for hiding this comment

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

go fmt

enum = [
enum = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

go fmt

client/team.go Outdated
Comment on lines 8 to 10
}
// AREL TODO: Check this
if payload.OrganizationId != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops

Comment on lines 72 to 73
createTeamPayload := TeamCreatePayload{}
copier.Copy(&createTeamPayload, &mockTeam)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why creating a copy? isn't it immutable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +28 to +29
var result Team
err := self.http.Get("/teams/"+id, nil, &result)
Copy link
Contributor

@liranfarage89 liranfarage89 Aug 30, 2021

Choose a reason for hiding this comment

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

a little out of scope, but why there is a diff between get-team to get-teams in our API? is there a special reason that you know?
In general I expected to see /team?id={id}&orgId{org_id}... where an empty id triggers get-all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a design choice we've made. I think it's technically more RESTful but I'm not sure.
One difference, for example, is that the by-id API returns a single team (not an array), and returns 404 if there's no such team

Comment on lines 38 to 40
Describe("Teams", func() {
var returnedTeams []Team
mockTeams := []Team{mockTeam}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to add validation tests? to those not_empty conditions..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HeverFarber
Copy link
Contributor

@RLRabinowitz do you want to add integration tests also?

Copy link
Contributor

@liranfarage89 liranfarage89 left a comment

Choose a reason for hiding this comment

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

Nice work 👍 . maybe ask for another 👀 , since that it's the first time I'm reviewing go in general and our provider in particular.

Name: "team-name",
}

Describe("Team", func() {
Copy link
Member

Choose a reason for hiding this comment

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

Get Team

Copy link
Contributor Author

Choose a reason for hiding this comment

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

})
})

Describe("Teams", func() {
Copy link
Member

Choose a reason for hiding this comment

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

Get All Teams

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RLRabinowitz RLRabinowitz added ready to merge PR approved - can be merged once the PR owner is ready and removed pending final review labels Aug 30, 2021
@RLRabinowitz RLRabinowitz merged commit 4af7b89 into main Aug 30, 2021
@RLRabinowitz RLRabinowitz deleted the chore-add-team-api-client branch August 30, 2021 14:44
@RLRabinowitz
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge PR approved - can be merged once the PR owner is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants