Skip to content
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

Make declarative jobsets a lot more usable #814

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dasJ
Copy link
Member

@dasJ dasJ commented Sep 13, 2020

  • Document the fields
  • Default most fields
  • Mostly validate fields
  • Allow rich values for enabled and type
  • Print all errors instead of only the first

Errors look like this:

image

@grahamc
Copy link
Member

grahamc commented Mar 8, 2021

Beyond the merge conflicts present, I think this PR would be easier to review and merge if the validation of the data and fields were its own function, with tests written to cover at least its most important cases.

I've rebased the rest of it (docs, the migration) in the attached patch, hopefully that helps:
0001-Partially-rebase-814-to-master.patch.txt

@dasJ dasJ force-pushed the feat/better-decl-jobsets branch 2 times, most recently from 3d17229 to 0e4d2e7 Compare March 9, 2021 16:02
@dasJ dasJ marked this pull request as draft March 9, 2021 16:47
@dasJ dasJ force-pushed the feat/better-decl-jobsets branch from 0e4d2e7 to 2c1b712 Compare March 9, 2021 16:56
@dasJ dasJ marked this pull request as ready for review March 9, 2021 16:57
@dasJ
Copy link
Member Author

dasJ commented Mar 9, 2021

cc @grahamc as your watch was probably dismissed by me marking this PR as a draft

@dasJ
Copy link
Member Author

dasJ commented Mar 11, 2021

@grahamc Since this also fixes a bug I introduced, do you want me to make a PR with only that bugfix so it gets to master faster or does this have the change to be merged soon?

@grahamc
Copy link
Member

grahamc commented Mar 11, 2021 via email

@grahamc
Copy link
Member

grahamc commented Mar 16, 2021

I pulled the fix for #889 out in to #891, since I'd really like to see the validation here made in to its own function.

- Document the fields
- Default most fields
- Mostly validate fields
- Allow rich values for `enabled` and `type`
- Print all errors instead of only the first
- Fix a bug in the project name PR that i recently introduced
@dasJ dasJ force-pushed the feat/better-decl-jobsets branch from 2c1b712 to e2797df Compare July 11, 2021 08:46
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.

None yet

2 participants