-
Notifications
You must be signed in to change notification settings - Fork 31
feat: deployments with builds #471
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: deployments with builds #471
Conversation
Codecov Report
@@ Coverage Diff @@
## master #471 +/- ##
==========================================
+ Coverage 50.98% 51.15% +0.17%
==========================================
Files 149 149
Lines 11218 11291 +73
==========================================
+ Hits 5719 5776 +57
- Misses 5300 5316 +16
Partials 199 199
|
97c5548 to
b12dde2
Compare
jbrockopp
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.
Looks like there are conflicts in the go.sum file.
Could we get those resolved?
https://github.com/go-vela/server/pull/471/conflicts
go.mod
Outdated
| @@ -1,5 +1,7 @@ | |||
| module github.com/go-vela/server | |||
|
|
|||
| replace github.com/go-vela/types => github.com/JayCeeJr/types v0.9.1-beta | |||
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.
Should this line be removed?
Looks like go-vela/types#193 was merged 👍
So I think you could vendor in the latest changes from that repo instead of this line.
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'll pull that line out
jbrockopp
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.
LGTM
jbrockopp
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.
LGTM
matt-fevold
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.
LGTM, minor question on why 3 builds and not say 1 or 10, but that's just my own curiosity. also looks like there's more go.mod conflicts so once those are resolved I'll approve.
| Table(constants.TableBuild). | ||
| Select("*"). | ||
| Where(filters). | ||
| Limit(3). |
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.
what's the motivation behind 3?
matt-fevold
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.
LGTM
jbrockopp
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.
LGTM
Return the last 3 builds related to the deployment in the deployment list.
Related Issue: go-vela/community#364
Depends on: go-vela/types#193