-
Notifications
You must be signed in to change notification settings - Fork 30
refactor(api): move build logic to separate package #864
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #864 +/- ##
==========================================
+ Coverage 66.13% 71.72% +5.59%
==========================================
Files 297 304 +7
Lines 13876 12794 -1082
==========================================
Hits 9177 9177
+ Misses 4252 3170 -1082
Partials 447 447
|
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| // swagger:operation GET /api/v1/repos/{org}/{repo}/builds/{build}/token builds GetBuildToken |
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 wonder if this endpoint should be in the api/auth package instead of api/build since it is auth-specific. Then all of the auth stuff is in the same package even if the API spreads it across a variety of endpoints. What do you think?
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.
Yeah I was thinking similarly, but landed on sticking with the build package since the URL path is very much of the build flavor.
|
|
||
| // getPRNumberFromBuild is a helper function to | ||
| // extract the pull request number from a Build. | ||
| func getPRNumberFromBuild(b *library.Build) (int, 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.
this is used for both CreateBuild and UpdateBuild but is in create.go - should it go in a file on its own? 🤔
It's probably fine here, but I'm asking anyway. 🤷
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.
Going to leave it there for now, but I almost wonder if this belongs in types rather than as a helper function
cognifloyd
left a comment
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.
Some misc questions and a couple typos.
plyr4
left a comment
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.
nice
Based off of #615 #754 #809 #828 #829 #847 #848 #849 #851 #853 #855 and #856
This change continues the refactor efforts initially introduced in the above PRs.
This adds a new
buildpackage to thegithub.amrom.workers.dev/go-vela/server/apipackage.This contains all the same handlers that existed previously but with each of them within their own file.
It also adds a new
webhookpackage in order to exportPublishToQueueas a function withinbuildpackage.Now that build has been moved to its own package, I was also able to put
PlanStepsandPlanServicesin their respective packages.