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

feat: worker auth endpoints #212

Merged
merged 12 commits into from
Mar 24, 2023
Merged

feat: worker auth endpoints #212

merged 12 commits into from
Mar 24, 2023

Conversation

plyr4
Copy link
Contributor

@plyr4 plyr4 commented Mar 17, 2023

this adds the server functionality introduced in go-vela/server#790
it also cleans up a couple tests

@plyr4 plyr4 requested a review from a team as a code owner March 17, 2023 22:18
@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Merging #212 (b3a0455) into main (e58ef26) will increase coverage by 0.66%.
The diff coverage is 100.00%.

❗ Current head b3a0455 differs from pull request most recent head 6aa276a. Consider uploading reports for the commit 6aa276a to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #212      +/-   ##
==========================================
+ Coverage   90.75%   91.42%   +0.66%     
==========================================
  Files          18       18              
  Lines        1331     1365      +34     
==========================================
+ Hits         1208     1248      +40     
+ Misses         91       87       -4     
+ Partials       32       30       -2     
Impacted Files Coverage Δ
vela/build.go 97.08% <ø> (ø)
vela/admin.go 97.41% <100.00%> (+0.35%) ⬆️
vela/authentication.go 82.78% <100.00%> (+1.20%) ⬆️
vela/client.go 88.62% <100.00%> (+2.11%) ⬆️
vela/worker.go 100.00% <100.00%> (ø)

vela/authentication_test.go Show resolved Hide resolved
vela/authentication_test.go Outdated Show resolved Hide resolved
@plyr4 plyr4 marked this pull request as draft March 21, 2023 15:16
vela/admin_test.go Show resolved Hide resolved
vela/worker_test.go Show resolved Hide resolved
@plyr4 plyr4 marked this pull request as ready for review March 23, 2023 21:45
@plyr4 plyr4 force-pushed the feat/worker-auth branch from b3a0455 to 6aa276a Compare March 23, 2023 22:15
Copy link
Contributor

@ecrupper ecrupper left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

LGTM. I have one question about why we need some functions, but that doesn't block merging this.

@@ -300,6 +351,24 @@ func ExampleWorkerService_Add() {
fmt.Printf("Received response code %d, for worker %+v", resp.StatusCode, worker)
}

func ExampleWorkerService_RefreshAuth() {
Copy link
Member

Choose a reason for hiding this comment

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

Where is this used? Same goes for the rest of the ExampleWorkerService_* funcs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

honestly im not sure. i added it for consistency

Copy link
Contributor Author

@plyr4 plyr4 Mar 24, 2023

Choose a reason for hiding this comment

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

seemed like leftover boiler plate tests from the initial worker router commits. we can probably remove them in favor the more detailed tests above them

Copy link
Member

@wass3rw3rk wass3rw3rk Mar 24, 2023

Choose a reason for hiding this comment

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

this is used for documentation, see https://pkg.go.dev/github.com/go-vela/[email protected]/vela#example-WorkerService.Update as an example (no pun intended) which is extracted from ExampleWorkerService_Update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh!!! 😮‍💨 thats awesome. i didnt know go doc used test naming to populate that. thanks @wass3rw3rk

@plyr4 plyr4 merged commit afa7fa4 into main Mar 24, 2023
@plyr4 plyr4 deleted the feat/worker-auth branch March 24, 2023 17:04
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.

4 participants