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.

Sign up for GitHub

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

Jump to bottom

xtrigger sequence fix - master #3287

Merged
merged 8 commits into from
Aug 29, 2019
Merged

xtrigger sequence fix - master #3287

merged 8 commits into from
Aug 29, 2019

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Aug 12, 2019 •
edited
Loading

master companion of #3285

These changes (along with #3285) close #3283

TODO:

  • some basic Python 3 upgrade (e.g. format strings)
  • fix some xtrigger unit tests

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • No documentation update required.

@hjoliver hjoliver added the bug label Aug 12, 2019
@hjoliver hjoliver added this to the cylc-8.0a1 milestone Aug 12, 2019
@hjoliver hjoliver self-assigned this Aug 12, 2019
matthewrmshin
matthewrmshin suggested changes Aug 12, 2019
View reviewed changes
Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

Similar comments...

cylc/flow/xtrigger_mgr.py Outdated Show resolved Hide resolved
tests/xtriggers/03-sequence.t Show resolved Hide resolved
tests/xtriggers/03-sequence.t Outdated Show resolved Hide resolved
@cylc cylc deleted a comment Aug 14, 2019
@hjoliver hjoliver mentioned this pull request Aug 14, 2019
Merged
6 tasks
@cylc cylc deleted a comment Aug 14, 2019
@cylc cylc deleted a comment Aug 14, 2019
@cylc cylc deleted a comment Aug 14, 2019
matthewrmshin
matthewrmshin reviewed Aug 23, 2019
View reviewed changes
Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

LGTM now. Any more update on this?

@hjoliver
Copy link
Member Author

This branch still needs a change log entry, and several unit tests have failed - should be easy to fix as soon as I have time to get back to it.

@cylc cylc deleted a comment Aug 28, 2019
@hjoliver hjoliver marked this pull request as ready for review August 28, 2019 07:35
@hjoliver
Copy link
Member Author

This should now be good to go @matthewrmshin (if tests pass...).

Pinging @kinow for 2nd review, as he helped out with the unit tests (thanks for that 👍!)

@hjoliver
Copy link
Member Author

(Note #3285 - same change on 7.8.x - already merged).

kinow
kinow suggested changes Aug 28, 2019
View reviewed changes
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

I think Travis is unhappy with this change. Failed all 4 function test jobs. Found a few minor issues with the IDE or reading the change in GitHub. Looks good otherwise 🎉 ready to approve once Travis build passes.

cylc/flow/taskdef.py Outdated Show resolved Hide resolved
cylc/flow/taskdef.py Outdated Show resolved Hide resolved
cylc/flow/xtrigger_mgr.py Outdated Show resolved Hide resolved
cylc/flow/xtrigger_mgr.py Show resolved Hide resolved
cylc/flow/scheduler.py Outdated Show resolved Hide resolved
cylc/flow/config.py Show resolved Hide resolved
cylc/flow/task_pool.py Show resolved Hide resolved
cylc/flow/tests/test_config.py Show resolved Hide resolved
@cylc cylc deleted a comment Aug 28, 2019
hjoliver
hjoliver commented Aug 28, 2019
View reviewed changes
cylc/flow/scheduler.py Outdated Show resolved Hide resolved
@hjoliver
Copy link
Member Author

All feedback addressed and tests fixed (let's see if Travis CI agrees ...)

@cylc cylc deleted a comment Aug 28, 2019
matthewrmshin
matthewrmshin reviewed Aug 29, 2019
View reviewed changes
Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

Almost there. Some changes were lost in one of the tests?

tests/xtriggers/03-sequence.t Outdated Show resolved Hide resolved
tests/xtriggers/03-sequence.t Show resolved Hide resolved
@hjoliver
Copy link
Member Author

Ah crap, good spotting. I suspect I missed a commit between work and home computers, and did a force push.

@hjoliver
Copy link
Member Author

(Re)fixed.

@cylc cylc deleted a comment Aug 29, 2019
@matthewrmshin matthewrmshin merged commit 4b27534 into cylc:master Aug 29, 2019
@hjoliver hjoliver deleted the xtrigger-sequence-fix-c8 branch August 29, 2019 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kinow kinow kinow approved these changes

@matthewrmshin matthewrmshin matthewrmshin approved these changes

Assignees

@hjoliver hjoliver

Labels
bug
Projects
None yet
Milestone
cylc-8.0a1
Development

Successfully merging this pull request may close these issues.

xtriggers not sequence-specific
3 participants
@hjoliver @kinow @matthewrmshin