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

Remove remaining Cylc Review files #3024

Merged
merged 2 commits into from
Mar 21, 2019

Conversation

kinow
Copy link
Member

@kinow kinow commented Mar 21, 2019

Complement to 3f7a601, removes the remaining files related to Cylc Review, as well as documentation mentioning it.

Checked if TASK_STATUS_GROUPS was used with the IDE, then with grep, and finally confirmed where it was used before with ol': git log -S "TASK_STATUS_GROUPS" --patch.

I think it should be safe to remove it too, as not used in the current code base. Hopefully not breaking anything in Travis-CI. Suite five and all unit tests running OK locally.

…view, as well as documentation mentioning it.
@kinow
Copy link
Member Author

kinow commented Mar 21, 2019

I believe this pull request is Cylc 8 only, without the need to backport to 7.8 branch.

@kinow kinow added this to the cylc-8.0a1 milestone Mar 21, 2019
@kinow kinow self-assigned this Mar 21, 2019
@kinow
Copy link
Member Author

kinow commented Mar 21, 2019

Found while reviewing External Triggers following up discussion in Riot.

@kinow kinow added small doc Documentation labels Mar 21, 2019
Copy link
Collaborator

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

I've reviewed & I am happy, so would otherwise approve & merge now, but I have noticed one other method we can drop, & it would be cleanest & easiest to include in this PR. If you don't mind can you update your code changes here to remove it, then I will approve & merge immediately?

@@ -105,25 +105,6 @@
# set CYLC_TEST_HOST and CYLC_TEST_OWNER for remote job tests.
# port_is_busy $PORT
# Return 0 if $PORT is busy or 1 if $PORT is not busy.
# cylc_ws_init $NS $UTIL
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can also now get rid of the port_is_busy method, which was only used across the test suite inside this cylc_ws_init method. (And now I have noticed it, we should drop the # tags on new line 580, just above where that method is defined, which was clearly for the aid of some developer who neglected to remove it. From git blame I can see that the developer in question was in fact myself 😬.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good spot, thanks for reviewing it so quickly. Updating PR in a few moments. Thanks @sadielbartholomew !

Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully removed the right lines here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent, thanks. Given the time in NZ I imagine you are no longer online so I have given two Travis CI jobs that were failing on flaky tests a kick. Once they pass I'll get this merged.

Copy link
Collaborator

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

Great work. Over 800 lines of clutter removed! All checks now pass. (For the cylc-8 branch only as 7.8.x still has the Review service.)

@sadielbartholomew sadielbartholomew merged commit 1ac175c into cylc:master Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants