-
Notifications
You must be signed in to change notification settings - Fork 28
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(types): add support for schedules #833
Conversation
Codecov Report
@@ Coverage Diff @@
## main #833 +/- ##
==========================================
+ Coverage 57.49% 58.11% +0.62%
==========================================
Files 271 274 +3
Lines 15892 16129 +237
==========================================
+ Hits 9137 9374 +237
Misses 6331 6331
Partials 424 424
|
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 — I like the types move + nested API objects. Step in the right direction for sure.
@jbrockopp Out of curiosity, what was the rationale for the switch from |
@ecrupper Great question and apologies I didn't highlight the reasoning behind that change. The tl;dr is github.com/robfig/cron doesn't support all the functionality we need for this feature. We need to be able to capture both the next (upcoming) time to run for an entry as well as the previous time. There is an open PR to add support for this functionality to that library: However, it has been > 2 years since that PR was opened and haven't seen much activity on it since. Also, that library appears to be abandoned since it has been > 2 years since the last commit: |
You can see the code that accomplishes this within https://github.com/go-vela/server/pull/836/files#diff-dfa7912983d4dacc511bc4724e878cf969898ce5db7f21bd1f2918c034d2466aR212-R227. |
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.
love the schedule stuff that's starting to flow in 😍
i have a bit of a concern with the approach though. it seems we are entering a bit of "frankenstein" territory which is making it harder to keep up with what is in what state.
the feature addition also kicks off two bigger refactors (nested objects and types move which are fine things on their own). however, now we will basically have 3 bigger refactors in some kind of transitional phase (nested objects, types move, plus db/api refactors you've been kindly working through).
personally, it would be more palatable if this happened in the types and server repos as appropriate in the current state and we work through the types move and nested objects stuff separately, but open to other ideas.
any thoughts on that?
@wass3rw3rk this is understandable and I'll begin efforts to implement code changes based off current state. However, since you asked, the reason why I'm trying to bundle multiple changes into one is based off timing. Historically, the amount of time it takes to get PRs reviewed and merged hasn't been fast. That's natural with opensource projects, but to your point, the DB refactor would already be done if this cycle was faster. This is the exact reason why I'm trying to do both API and DB in parallel to improve the rate of change. |
Closing in favor of go-vela/types#289 |
@jbrockopp thanks for the understanding and feedback. we are trying to improve that on all fronts. |
xref: go-vela/community#538
Part of go-vela/community#772
This change adds the
structs
(a.k.a. types) forschedules
to Vela.This adds a new
types
package to the following packages:github.com/go-vela/server/api
- code that would normally be ingithub.com/go-vela/types/library
github.com/go-vela/server/database
- code that would normally be ingithub.com/go-vela/types/database
I'm currently piggy backing off of a previous repo restructure proposal for this.
The end goal is to remove https://github.com/go-vela/types in favor of code within this repo.
Also, you'll find that this implementation of
types
for schedules also incorporates this proposal:go-vela/community#639