Skip to content

Conversation

@JordanSussman
Copy link
Collaborator

@JordanSussman JordanSussman commented May 3, 2023

xref: go-vela/community#538

Part of go-vela/community#772

Dependent on #833 and #834

The code that manages the execution of schedules will added in a separate code change.

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Merging #836 (d601eba) into main (3919d6c) will decrease coverage by 1.09%.
The diff coverage is 10.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #836      +/-   ##
==========================================
- Coverage   62.18%   61.09%   -1.09%     
==========================================
  Files         281      290       +9     
  Lines       15111    15434     +323     
==========================================
+ Hits         9397     9430      +33     
- Misses       5271     5558     +287     
- Partials      443      446       +3     
Impacted Files Coverage Δ
api/schedule/delete.go 0.00% <0.00%> (ø)
api/schedule/get.go 0.00% <0.00%> (ø)
api/schedule/list.go 0.00% <0.00%> (ø)
api/schedule/update.go 0.00% <0.00%> (ø)
router/middleware/schedule/schedule.go 0.00% <0.00%> (ø)
api/schedule/create.go 8.87% <8.87%> (ø)
router/middleware/allowlist_schedule.go 100.00% <100.00%> (ø)
router/middleware/schedule/context.go 100.00% <100.00%> (ø)
router/middleware/schedule_frequency.go 100.00% <100.00%> (ø)

JordanSussman and others added 6 commits May 19, 2023 09:23
Co-authored-by: Jordan Brockopp <[email protected]>
Co-authored-by: Jordan Brockopp <[email protected]>
Co-authored-by: Jordan Brockopp <[email protected]>
Co-authored-by: Jordan Brockopp <[email protected]>
Co-authored-by: Jordan Brockopp <[email protected]>
Co-authored-by: Jordan Brockopp <[email protected]>
@JordanSussman
Copy link
Collaborator Author

@jbrockopp @ecrupper feedback has been addressed.

jbrockopp
jbrockopp previously approved these changes May 19, 2023
Copy link
Contributor

@jbrockopp jbrockopp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ecrupper
Copy link
Contributor

I also want to point out that I was kind of disappointed with github.com/adhocore/gronx error handling with invalid entries. For example, I provided an entry of 0 0 0 * *, which is invalid due to day_of_month being 0, but the error output was

{
	"error": "schedule of  is invalid: tried so hard"
}

I expect users to be able to diagnose an invalid cron expression, but "tried so hard" isn't exactly a great push in the right direction.

}

// send API call to capture the updated schedule
s, _ = database.FromContext(c).GetScheduleForRepo(r, dbSchedule.GetName())
Copy link
Contributor

@plyr4 plyr4 May 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im not sure why we ignore the return errors on so many database calls across the codebase.
(im not calling you out, im just curious if yall have thoughts)

we dont have to make this change now, but im wondering why we continue to follow that pattern?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be honest that I didn't think about handling the error since so much of the existing code base does not... How about we open up a separate GitHub issue to track updating all of these database calls?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW -

We should plan to have the Create* and Update* funcs return the type so we don't need these calls

i.e. #701

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoever wrote that PR needs to get on it

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.

Nice

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