Add dep api token crud #1564

aliotta · 2024-02-22T15:05:10Z

Description

Describe the purpose of this pull request.

Adds the ability to manage deployment api tokens including Creation, Removal, Updating the apiToken role, and Listing.

🎟 Issue(s)

Related #18928

🧪 Functional Testing

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

Use the cli to create, update, remove and delete deployment api tokens passing in the flags that are required and testing both passing in and not passing in the optional flags

📸 Screenshots

📋 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

codecov · 2024-02-26T21:37:39Z

Codecov Report

Attention: Patch coverage is 92.46032% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 86.51%. Comparing base (81b3793) to head (e648de4).
Report is 4 commits behind head on main.

Files	Patch %	Lines
cloud/apitoken/apitoken.go	90.44%	8 Missing and 7 partials ⚠️
cmd/cloud/deployment.go	95.78%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1564      +/-   ##
==========================================
+ Coverage   86.41%   86.51%   +0.09%     
==========================================
  Files         113      114       +1     
  Lines       15668    15918     +250     
==========================================
+ Hits        13540    13771     +231     
- Misses       1280     1290      +10     
- Partials      848      857       +9

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

vandyliu · 2024-02-26T22:41:58Z

cloud/apitoken/apitoken.go

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


@sunkickr @kushalmalani
do we normally have this here and if so do we want to keep doing it like this?
I'm wondering since it would be meaningless for the user to see, but I guess it would be useful for us if we need to debug

I think most API calls have this but I have never seen it actually come up

Yeah would be useful for debugging. Can we improve that error message though? it looks like a debugging log

tbh it seems unecessary to me as the method that calls it prints out a prettier error message

e.g.

shrividyahegde@Hegde ~ % astro deployment user update --deployment-id clt6yb6vm001r01nmwzffhbs9
Enter a user Deployment role or custom role name to update user: sample

error in NormalizeAPIError
Error: Invalid request: Role sample does not exist
shrividyahegde@Hegde ~ %

I removed them

vandyliu · 2024-02-26T22:43:00Z

cloud/apitoken/apitoken.go

+	}
+
+	if apiTokenID == "" {
+		// Get all org apiTokens. Setting limit to 1000 for now


Suggested change

// Get all org apiTokens. Setting limit to 1000 for now

// Get all deployment apiTokens. Setting limit to 1000 for now

vandyliu · 2024-02-26T22:45:38Z

@shri-astro @sreenusuuda
Just a heads up for more QA that will need to be done for adding deployment tokens to CLI

sunkickr · 2024-02-28T14:36:46Z

Why do we call it remove instead of delete? The api token is being deleted correct? Remove makes it sound like the token is being removed from the deployment but still exists

aliotta · 2024-02-28T16:31:37Z

Why do we call it remove instead of delete? The api token is being deleted correct? Remove makes it sound like the token is being removed from the deployment but still exists

Good call. That verbiage was a holdover from remove user/team from deployment (where I was riffing from) but does not fit here. I will update to delete.

aliotta · 2024-02-29T14:15:51Z

Why do we call it remove instead of delete? The api token is being deleted correct? Remove makes it sound like the token is being removed from the deployment but still exists

done

kushalmalani · 2024-03-04T07:32:01Z

cmd/cloud/deployment.go

+			return createDeploymentAPIToken(cmd, nil, out)
+		},
+	}
+	cmd.Flags().StringVarP(&apiTokenRole, "role", "r", "", "The role for the "+


Should DEPLOYMENT_ADMIN be default role?

kushalmalani · 2024-03-04T07:44:18Z

cloud/apitoken/apitoken.go

+		DynamicPadding: true,
+		Header:         []string{"NAME", "DESCRIPTION", "ID", "DEPLOYMENT ROLE", "CREATE DATE", "UPDATE DATE"},
+	}
+	apiTokens, err := GetDeploymentAPITokens(client, deployment, apiTokenPagnationLimit)


if there are no api tokens, we should display an error message, rather than an empty table with only headers

kushalmalani · 2024-03-04T07:47:45Z

cloud/apitoken/apitoken.go

+		Name:        name,
+		Description: &description,
+	}
+	resp, err := client.CreateDeploymentApiTokenWithResponse(httpContext.Background(), ctx.Organization, deployment, mutateAPITokenInput)


we should save the api token from response to ASTRO_API_TOKEN variable. Right now, the token gets created, but user cannot use the token value

will get a pr up for your comments

I think instead of setting it we really want to print it to screen so they can use it where they want to.

worried about the token leaking in ci cd logs...

aliotta added 15 commits February 14, 2024 17:08

add cmds for managing deployment user/teams

a2824d0

add unit testing and fix user select bug

0b52477

add unit test to cmd files

c05c266

Merge branch 'main' into userAndTeamDeploymentRoleManagement

7bc25dc

lint

1b985fe

fix default deployment role in add command

f8e7fe5

wip

020d735

wip

6a480ed

add crud methods for managing deployment api tokens

33b68e3

cleanup

0c3cbc0

wip

3eaf09b

add tests for cmd

53e66de

wip

733d74b

add tests to service layer

7bc5277

Merge branch 'main' into addDepApiTokenCrud

7ae0c5d

aliotta marked this pull request as ready for review February 26, 2024 20:43

aliotta requested review from neel-astro, kushalmalani, sunkickr and vandyliu as code owners February 26, 2024 20:43

aliotta added 5 commits February 26, 2024 15:44

cleanup

4b7f425

cleanup

af518d2

lint

3b12cbe

run go fumpt

0f939da

go fumpt all files in need

79c6718

vandyliu reviewed Feb 26, 2024

View reviewed changes

remove uneeded comment

e91a8b1

replace remove with delete

443e0bf

remove uneeded print lines

e648de4

aliotta requested a review from vandyliu February 29, 2024 14:21

vandyliu approved these changes Feb 29, 2024

View reviewed changes

aliotta merged commit 72d98da into main Feb 29, 2024
4 of 5 checks passed

aliotta deleted the addDepApiTokenCrud branch February 29, 2024 22:00

kushalmalani reviewed Mar 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dep api token crud #1564

Add dep api token crud #1564

aliotta commented Feb 22, 2024 •

edited

codecov bot commented Feb 26, 2024 •

edited

vandyliu Feb 26, 2024

sunkickr Feb 28, 2024

kushalmalani Feb 28, 2024

aliotta Feb 29, 2024 •

edited

aliotta Feb 29, 2024

vandyliu Feb 26, 2024

vandyliu commented Feb 26, 2024

sunkickr commented Feb 28, 2024

aliotta commented Feb 28, 2024 •

edited

aliotta commented Feb 29, 2024

kushalmalani Mar 4, 2024

kushalmalani Mar 4, 2024

kushalmalani Mar 4, 2024

aliotta Mar 15, 2024

aliotta Mar 15, 2024

kushalmalani Mar 15, 2024

	// Get all org apiTokens. Setting limit to 1000 for now
	// Get all deployment apiTokens. Setting limit to 1000 for now

Add dep api token crud #1564

Add dep api token crud #1564

Conversation

aliotta commented Feb 22, 2024 • edited

Description

🎟 Issue(s)

🧪 Functional Testing

📸 Screenshots

📋 Checklist

codecov bot commented Feb 26, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliotta Feb 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vandyliu commented Feb 26, 2024

sunkickr commented Feb 28, 2024

aliotta commented Feb 28, 2024 • edited

aliotta commented Feb 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliotta commented Feb 22, 2024 •

edited

codecov bot commented Feb 26, 2024 •

edited

aliotta Feb 29, 2024 •

edited

aliotta commented Feb 28, 2024 •

edited