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

cylc-insert: prevent insertion at invalid cycle point. #2107

Conversation

oliver-sanders
Copy link
Member

Close #1239

Don't insert a task unless its cycle point is on one of the tasks sequences. Over-ridable by the --no-check option.

@oliver-sanders oliver-sanders added the bug? Not sure if this is a bug or not label Jan 11, 2017
@oliver-sanders oliver-sanders added this to the next release milestone Jan 11, 2017
@oliver-sanders oliver-sanders self-assigned this Jan 11, 2017
@oliver-sanders oliver-sanders force-pushed the 1239.dont-insert-at-ungraphed-cycle-points branch 2 times, most recently from 467456e to 7908ed2 Compare January 11, 2017 12:28
if not isinstance(items, list):
items = [items]
if stop_point_string == "None":
stop_point_string = None
return self._put("insert_tasks", (items,),
{"stop_point_string": stop_point_string})
{"stop_point_string": stop_point_string,
"no_check": True if no_check == 'True' else False})
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be sufficient to do: "no_check": no_check == 'True'.

But then why would no_check be a string?

Copy link
Member Author

Choose a reason for hiding this comment

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

Objects serialised for transmission on the comms layer? See line 154 in the diff above where stop_point_string is treated in a similar way.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I forgot it was done by urllib.urlencode not json.dumps.

@oliver-sanders oliver-sanders force-pushed the 1239.dont-insert-at-ungraphed-cycle-points branch from 7908ed2 to 8daa468 Compare January 11, 2017 14:21
@oliver-sanders oliver-sanders force-pushed the 1239.dont-insert-at-ungraphed-cycle-points branch from 8daa468 to 525208d Compare January 12, 2017 09:34
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Good, tested.

@hjoliver
Copy link
Member

(I guess you just rebased the branch; I'll wait on Travis CI before merging...)

@hjoliver
Copy link
Member

Huh, Travis CI doesn't like tests/restart/19-checkpoint.t. It passes in my environment (and I updated my remote branch after the rebase).

@matthewrmshin
Copy link
Contributor

tests/restart/19-checkpoint.t has become a bit sensitive lately (mainly in my environment, but it looks like Travis CI is now affected). I don't think the failure is related to this change. I'll have a look at the test again in due course.

@oliver-sanders
Copy link
Member Author

Passes in my environment.

@matthewrmshin
Copy link
Contributor

(I have bundled a change in #2100 to improve the stability of tests/restart/19-checkpoint.t.)

@hjoliver
Copy link
Member

In that case, merging now...

@hjoliver hjoliver merged commit bd725ee into cylc:master Jan 13, 2017
@oliver-sanders oliver-sanders deleted the 1239.dont-insert-at-ungraphed-cycle-points branch May 23, 2017 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug? Not sure if this is a bug or not
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants