-
Notifications
You must be signed in to change notification settings - Fork 14
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: Assign teams a project resource #140
Conversation
…into feat-team-project-assignment � Conflicts: � env0/provider.go
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.
Greate job, see some questions and comments
if payload.TeamId == "" { | ||
return TeamProjectAssignment{}, errors.New("must specify team_id") | ||
} | ||
if payload.ProjectRole == "" || |
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.
just asking, payload.ProjectRole == ""
is not enough?
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.
nope.. I tried that but since it's a string (typedef of a string) it could be any string.
const dummyProjectRole = "Admin" | ||
const dummyTeamId = "dummyTeamId" | ||
|
||
var _ = Describe("TeamProjectAssignment", func() { |
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.
Where are the tests for your argument's validations? like missing field you have checked
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.
Describe("Failure", func() { |
Type: schema.TypeString, | ||
Description: "id of the team", | ||
Required: true, | ||
ForceNew: true, |
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.
what ForceNew
do?
do we need it?
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.
ForceNew indicates that any change in this field requires the resource to be destroyed and recreated.
https://www.terraform.io/docs/extend/schemas/schema-behaviors.html#forcenew
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.
in other words, we want to destroy the existing teamProjectAssignment
when the teamId was changed.
if assignment.Id == id { | ||
d.Set("project_id", assignment.ProjectId) | ||
d.Set("team_id", assignment.TeamId) | ||
d.Set("role", assignment.ProjectRole) |
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.
should break
after found?
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.
also, should we use SetId
in this method?
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.
should
break
after found?
yes 7992743
also, should we use
SetId
in this method?
nope, we already have it ( they are equal...)
@@ -0,0 +1 @@ | |||
{} |
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.
?
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.
empty output... we don't print anything
Co-authored-by: Raz Ben Simon <raz.bensimon@env0.com>
…to feat-team-project-assignment
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.
🐐
Issue & Steps to Reproduce / Feature Request
Solution