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 job title required for job configs #166

Merged
merged 5 commits into from
Nov 3, 2022
Merged

Make job title required for job configs #166

merged 5 commits into from
Nov 3, 2022

Conversation

andebor
Copy link
Contributor

@andebor andebor commented Oct 26, 2022

This should not be merged until all datahub configs have been migrated to use job title.

Fixes #165

@andebor andebor added the draft label Oct 26, 2022
@@ -449,6 +449,9 @@ func (s *Scheduler) verify(jobConfiguration *JobConfiguration) error {
if len(jobConfiguration.Id) <= 0 {
return errors.New("job configuration needs an id")
}
if len(jobConfiguration.Title) <= 0 {
return errors.New("job configuration needs a title")
Copy link
Contributor

Choose a reason for hiding this comment

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

just a thought, maybe we should consider checking for duplicate titles as well. Not sure it this is the correct place - "addJob" is a good place to check for duplicates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. We currently do that in the deploy tool, but it would be nice to detect it here as well. Not everyone will use the deploy tool at all times, so we should just check it internally regardless. I'll have a look at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rompetroll I've added a check for unique title. Please have a look.

@andebor andebor removed the draft label Nov 2, 2022
@rompetroll rompetroll merged commit a907f18 into master Nov 3, 2022
@rompetroll rompetroll deleted the feature_165 branch November 3, 2022 07:00
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.

Make title required for job configurations
2 participants