-
Notifications
You must be signed in to change notification settings - Fork 13
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(deployment): add deployment table #323
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #323 +/- ##
==========================================
+ Coverage 96.12% 96.20% +0.07%
==========================================
Files 67 68 +1
Lines 7229 7370 +141
==========================================
+ Hits 6949 7090 +141
Misses 205 205
Partials 75 75
|
release and other merges caused some conflicts, so you'll have to resolve that first |
also, we will want to add tests in /database |
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.
A few random things I noticed + thoughts on giving library.Deployment
its own Environment()
method? I can imagine that could be useful to some users.
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.
one suggestion.. got a little confused with ID and Number 😵💫 let me know what you think - looks like Easton mentioned this also :disappear:
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 great.
RepoID *int64 `json:"repo_id,omitempty"` | ||
URL *string `json:"url,omitempty"` | ||
User *string `json:"user,omitempty"` |
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.
just curious, why'd we remove User?
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.
because I added a CreatedBy field instead
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
When a repo has a lot of deployments historically, viewing them in the UI is cumbersome and slow.
The loading of deployments on the deployments tab in Vela takes a few more seconds to load than the other pages (schedules, secrets, audits...) even when there are no deployments for a repo. This is due to the way deployments are stored and retrieved.
The changes made in this pull request add a new database table so that we can store deployment information instead of having to go to the scm to retrieve them. This (plus the implementation in server) greatly improve the time it takes for the deployments page to load.