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

User and team deployment role management #1553

Merged
merged 13 commits into from
Feb 21, 2024

Conversation

aliotta
Copy link
Contributor

@aliotta aliotta commented Feb 15, 2024

Description

Describe the purpose of this pull request.

Adds the ability to CRUD deployment users and deployment teams

🎟 Issue(s)

Related #18925 #18926

🧪 Functional Testing

List the functional testing steps to confirm this feature or fix.

Ran each method locally to verify it was working as expected

📸 Screenshots

Add screenshots to illustrate the validity of these changes.

📋 Checklist

  • Rebased from the main (or release if patching) branch (before testing)
  • Ran make test before taking out of draft
  • Ran make lint before taking out of draft
  • Added/updated applicable tests
  • Tested against Astro-API (if necessary).
  • Tested against Houston-API and Astronomer (if necessary).
  • Communicated to/tagged owners of respective clients potentially impacted by these changes.
  • Updated any related documentation

Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Attention: 40 lines in your changes are missing coverage. Please review.

Comparison is base (0105ca6) 86.28% compared to head (08873ac) 86.41%.

Files Patch % Lines
cloud/team/team.go 84.31% 12 Missing and 12 partials ⚠️
cloud/user/user.go 90.55% 6 Missing and 6 partials ⚠️
cmd/cloud/deployment.go 97.79% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1553      +/-   ##
==========================================
+ Coverage   86.28%   86.41%   +0.13%     
==========================================
  Files         113      113              
  Lines       15221    15668     +447     
==========================================
+ Hits        13133    13540     +407     
- Misses       1260     1280      +20     
- Partials      828      848      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cloud/team/team.go Outdated Show resolved Hide resolved
cloud/team/team.go Outdated Show resolved Hide resolved
cloud/team/team.go Outdated Show resolved Hide resolved
cloud/user/user.go Outdated Show resolved Hide resolved
cloud/user/user.go Outdated Show resolved Hide resolved
cmd/cloud/deployment.go Outdated Show resolved Hide resolved
cmd/cloud/deployment.go Outdated Show resolved Hide resolved
cmd/cloud/deployment.go Outdated Show resolved Hide resolved
cmd/cloud/deployment.go Outdated Show resolved Hide resolved
cmd/cloud/deployment.go Outdated Show resolved Hide resolved
}
var id string

// if an email was provided in the args we use it
Copy link
Contributor

Choose a reason for hiding this comment

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

comment seems incorrect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

if teams[i].Description != nil {
teamDescription = *teams[i].Description
}
for _, role := range *teams[i].Roles {
Copy link
Contributor

Choose a reason for hiding this comment

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

this scares me a lil if theres a bug on core side, then we might end up with nil pointer exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 protection added

users[i].FullName,
users[i].Username,
users[i].Id,
*users[i].DeploymentRole,
Copy link
Contributor

Choose a reason for hiding this comment

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

should we nil check here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 protection added

}
resp, err := client.MutateDeploymentUserRoleWithResponse(httpContext.Background(), ctx.Organization, deployment, userID, mutateUserInput)
if err != nil {
fmt.Println("error in MutateDeploymentUserRoleWithResponse")
Copy link
Contributor

Choose a reason for hiding this comment

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

should we be printing these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these are fine as is as we deff want to tell the user an error occurred.

}
err = astrocore.NormalizeAPIError(resp.HTTPResponse, resp.Body)
if err != nil {
fmt.Println("error in NormalizeAPIError")
Copy link
Contributor

Choose a reason for hiding this comment

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

should we be printing these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these are fine as is as we deff want to tell the user an error occurred.

}
if updateDeploymentRole == "" {
// no role was provided so ask the user for it
updateDeploymentRole = input.Text("Enter a user Deployment role or custom role name to update team: ")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
updateDeploymentRole = input.Text("Enter a user Deployment role or custom role name to update team: ")
updateDeploymentRole = input.Text("Enter a Deployment role or custom role name to update 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.

removed

Copy link
Contributor

@vandyliu vandyliu left a comment

Choose a reason for hiding this comment

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

thanks for the changes

@aliotta aliotta merged commit 6ee0185 into main Feb 21, 2024
4 of 5 checks passed
@aliotta aliotta deleted the userAndTeamDeploymentRoleManagement branch February 21, 2024 20:05
kushalmalani pushed a commit that referenced this pull request Feb 23, 2024
* add cmds for managing deployment user/teams

* add unit testing and fix user select bug

* add unit test to cmd files

* lint

* fix default deployment role in add command

* fix test name

* pr review comments addressed

* revert change to flag description for list deployments accross all worksapces

* add protections around nil pointers and fix some copy
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.

None yet

2 participants