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

Fix bugs in tutorial and CLI scheduling trigger #545

Merged
merged 5 commits into from Dec 1, 2022

Conversation

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Dec 1, 2022

I'd like your opinion on the extra call to ensure_storage_specs in make_schedule. If you go through the job route (with create_scheduling_job), it now gets called twice: once before creating the job (which makes sense to me, because you want to let the user know of any problems straight away), and another time when executing the job. Is that okay?

Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
…. no job)

Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x Flix6x added bug Something isn't working documentation Improvements or additions to documentation Scheduling CLI Still Needs Changelog Entry labels Dec 1, 2022
@Flix6x Flix6x added this to the 0.12.0 milestone Dec 1, 2022
@Flix6x Flix6x requested a review from nhoening December 1, 2022 13:15
@Flix6x Flix6x self-assigned this Dec 1, 2022
@nhoening
Copy link
Contributor

nhoening commented Dec 1, 2022

Actually, in my current refactoring PR, I go the same route, for the reason you mention. Maybe it's a bit cleaner there.

I worry that this PR and #537 will have quite some merge conflicts ...

@nhoening
Copy link
Contributor

nhoening commented Dec 1, 2022

But at least this one is small.

@coveralls
Copy link
Collaborator

coveralls commented Dec 1, 2022

Pull Request Test Coverage Report for Build 3595654316

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 64.98%

Totals Coverage Status
Change from base Build 3546393110: 0.003%
Covered Lines: 6587
Relevant Lines: 9517

💛 - Coveralls

Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x
Copy link
Contributor Author

Flix6x commented Dec 1, 2022

Doesn't need a changelog entry imo, because the bugs aren't part of any release.

@Flix6x Flix6x merged commit 236b472 into main Dec 1, 2022
@Flix6x Flix6x deleted the fix-bugs-in-tutorial-and-cli-scheduling-trigger branch December 1, 2022 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CLI documentation Improvements or additions to documentation Scheduling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants