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
Token Management Add/Remove/List/Update #1611
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1611 +/- ##
==========================================
+ Coverage 86.59% 86.77% +0.18%
==========================================
Files 114 114
Lines 16080 16661 +581
==========================================
+ Hits 13924 14458 +534
- Misses 1293 1315 +22
- Partials 863 888 +25 ☔ View full report in Codecov by Sentry. |
@@ -1,16 +1,20 @@ | |||
package workspace |
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.
I had to move this into a seperate file to prevent a cyclic import (caused by our auth package importing workspace and the new workspace token code importing organization)
req.Header.Add("x-astro-client-identifier", "cli") | ||
req.Header.Add("x-astro-client-version", version.CurrVersion) | ||
req.Header.Add("x-client-os-identifier", os+"-"+arch) | ||
req.Header.Add("User-Agent", fmt.Sprintf("astro-cli/%s", version.CurrVersion)) |
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.
API version is already handled on core API side so we don't need that info from the clients
cloud/deployment/deployment_token.go
Outdated
@@ -10,10 +10,14 @@ import ( | |||
"time" | |||
|
|||
astrocore "github.com/astronomer/astro-cli/astro-client-core" | |||
astrocoreiam "github.com/astronomer/astro-cli/astro-client-iam-core" |
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.
astrocoreiam "github.com/astronomer/astro-cli/astro-client-iam-core" | |
astroiamcore "github.com/astronomer/astro-cli/astro-client-iam-core" |
Just so we stay consistent
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.
done for all files
cloud/deployment/deployment_token.go
Outdated
@@ -10,10 +10,14 @@ import ( | |||
"time" | |||
|
|||
astrocore "github.com/astronomer/astro-cli/astro-client-core" | |||
astrocoreiam "github.com/astronomer/astro-cli/astro-client-iam-core" | |||
"github.com/astronomer/astro-cli/cloud/organization" | |||
workspace2 "github.com/astronomer/astro-cli/cloud/workspace-token" |
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.
Can we rename this? workspace2 sounds confusing
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.
done for all files
cloud/deployment/deployment_token.go
Outdated
if tokenTypes != nil { // verify the user has passed in an id that matches the operations expected token type | ||
if len(*tokenTypes) > 0 { |
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.
if tokenTypes != nil { // verify the user has passed in an id that matches the operations expected token type | |
if len(*tokenTypes) > 0 { | |
if tokenTypes != nil && len(*tokenTypes) > 0 { // verify the user has passed in an id that matches the operations expected token type |
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.
done for all files
cloud/deployment/deployment_token.go
Outdated
@@ -112,28 +121,20 @@ func CreateToken(name, description, role, deployment string, expiration int, cle | |||
} | |||
|
|||
// Update a deployment token | |||
func UpdateToken(id, name, newName, description, role, deployment string, out io.Writer, client astrocore.CoreClient) error { | |||
func UpdateToken(id, name, newName, description, role, deployment string, out io.Writer, client astrocore.CoreClient, iamClient astrocoreiam.CoreClient) error { |
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.
func UpdateToken(id, name, newName, description, role, deployment string, out io.Writer, client astrocore.CoreClient, iamClient astrocoreiam.CoreClient) error { | |
func UpdateToken(id, name, newName, description, role, deploymentID string, out io.Writer, client astrocore.CoreClient, iamClient astrocoreiam.CoreClient) error { |
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.
Same for all other functions too
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.
done for all files
cloud/deployment/deployment_token.go
Outdated
return nil | ||
} | ||
|
||
func UpsertWorkspaceTokenDeploymentRole(id, name, role, workspace, deployment, operation string, out io.Writer, client astrocore.CoreClient, iamClient astrocoreiam.CoreClient) error { |
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.
func UpsertWorkspaceTokenDeploymentRole(id, name, role, workspace, deployment, operation string, out io.Writer, client astrocore.CoreClient, iamClient astrocoreiam.CoreClient) error { | |
func UpsertWorkspaceTokenDeploymentRole(id, name, role, workspaceID, deploymentID, operation string, out io.Writer, client astrocore.CoreClient, iamClient astrocoreiam.CoreClient) error { |
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.
done for all files
if roles[i].Role == role { | ||
return errWorkspaceTokenInDeployment | ||
} else { | ||
continue |
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.
This is not needed.
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.
The continue is needed as line 614 would assign the old deployment role to the token instead of the role the user is assigning.
cloud/deployment/deployment_token.go
Outdated
if roles[i].EntityId == deployment { | ||
if roles[i].Role == role { |
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.
if roles[i].EntityId == deployment { | |
if roles[i].Role == role { | |
if roles[i].EntityId == deployment && roles[i].Role == role { |
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.
The continue is needed
cloud/deployment/deployment_token.go
Outdated
apiTokenDeploymentRoles := []astrocore.ApiTokenDeploymentRoleRequest{apiTokenDeploymentRole} | ||
roles := *token.Roles | ||
for i := range roles { | ||
if roles[i].EntityId == deployment { |
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.
Same comments as above function
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.
The continue is needed
cmd/cloud/deployment.go
Outdated
Short: "Update a Deployment or Organaization API token", | ||
Long: "Update a Deployment or Organaization API token that has a role in an Astro Deployment\n$astro workspace token update [TOKEN_ID] --name [new token name] --role [Possible values are DEPLOYMENT_ADMIN or a custom role name].", | ||
Short: "Update a Deployment API token", | ||
Long: "Update a Deployment API token that has a role in an Astro Deployment\n$astro workspace token update [TOKEN_ID] --name [new token name] --role [Possible values are DEPLOYMENT_ADMIN or a custom role name].", |
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.
Long: "Update a Deployment API token that has a role in an Astro Deployment\n$astro workspace token update [TOKEN_ID] --name [new token name] --role [Possible values are DEPLOYMENT_ADMIN or a custom role name].", | |
Long: "Update a Deployment API token that has a role in an Astro Deployment\n$astro deployment token update [TOKEN_ID] --name [new token name] --role [Possible values are DEPLOYMENT_ADMIN or a custom role name].", |
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.
good eyes
A bunch of the comments are applicable in other files too, so if you addressing them, please scan through all the files in this PR for similar changes. Since this PR is adding a lot of new commands, it would be helpful if you can post screenshots of them working against dev for more confidence |
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.
Thanks for addressing comments. Looks good
Description
Adds the ability to Add/Remove/List/Update the following
🎟 Issue(s)
Related #19973
🧪 Functional Testing
📸 Screenshots
📋 Checklist
make test
before taking out of draftmake lint
before taking out of draft