Skip to content

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%> (ø)

@plyr4 plyr4 marked this pull request as draft March 21, 2023 15:16
@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.

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.

5 participants