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

Authentication Washup & Fixed Cylc Scan #3509

Merged
merged 8 commits into from
May 5, 2020
Merged

Authentication Washup & Fixed Cylc Scan #3509

merged 8 commits into from
May 5, 2020

Conversation

datamel
Copy link
Contributor

@datamel datamel commented Mar 5, 2020 •
edited by hjoliver
Loading

Close #3449

Allow cylc scan to skip running suites which use previous authentication schemes rather than failing with traceback.

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).
  • Appropriate tests are included (unit and/or functional).
  • No change log entry required (why? e.g. invisible to users).

Notes to reviewers:

  • Cylc scan was failing due to a requirement for CYLC_SUITE_PUBLISH_PORT, which is not set in workflows run before Cylc 8.
  • Cylc scan now skips workflows run pre-cylc 8, rather than failing with traceback, skipped suites are listed in --debug mode.
  • I have changed a test (test_get_scan_items_from_fs_with_owner_active_only) which I believe was returning a pass incorrectly.

Closes #3516

@datamel datamel self-assigned this Mar 5, 2020
@datamel datamel added this to the cylc-8.0a2 milestone Mar 5, 2020
@datamel datamel mentioned this pull request Mar 11, 2020
Closed
oliver-sanders
oliver-sanders reviewed Mar 20, 2020
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Wow, what a simplification.

You know you are heading in the right direction when you can strip out 1600 lines of code and it still works.

I've found one thing which I think may break remote functionality but it's an easy fix.

cylc/flow/tests/network/test_client.py Show resolved Hide resolved
tests/authentication/03-full-read.t Show resolved Hide resolved
cylc/flow/suite_files.py Show resolved Hide resolved
cylc/flow/suite_files.py Show resolved Hide resolved
@datamel datamel requested a review from dpmatthews March 26, 2020 15:00
@datamel datamel force-pushed the authentication_washup_cylc_scan branch 3 times, most recently from 451f3d6 to 3047122 Compare April 16, 2020 15:03
@datamel
Copy link
Contributor Author

datamel commented Apr 16, 2020

Note that this change (with the removal of the contact 2 file) will break
./tests/cylc-message/00-ssh.t . This is currently set to skip but should probably be addressed in #3327.

@datamel datamel marked this pull request as ready for review April 16, 2020 16:46
@datamel datamel changed the title [WIP] - Fixed cylc scan, fixed test and added test Authentiation Washup & Fixed Cylc Scan Apr 16, 2020
@kinow kinow changed the title Authentiation Washup & Fixed Cylc Scan Authentication Washup & Fixed Cylc Scan Apr 17, 2020
oliver-sanders
oliver-sanders reviewed Apr 20, 2020
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders 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 this is all looking good, will test tomorrow.

cylc/flow/suite_files.py Show resolved Hide resolved
@datamel datamel force-pushed the authentication_washup_cylc_scan branch from 9f67aa9 to 69deb64 Compare April 29, 2020 15:00
@datamel datamel force-pushed the authentication_washup_cylc_scan branch from 69deb64 to 11d9a5e Compare April 30, 2020 16:18
@oliver-sanders oliver-sanders mentioned this pull request Apr 30, 2020
Closed
2 tasks
hjoliver
hjoliver approved these changes May 1, 2020
View reviewed changes
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.

LGTM, and tested as working 👍 One trivial suggestion.

cylc/flow/network/scan.py Outdated Show resolved Hide resolved
oliver-sanders
oliver-sanders reviewed May 1, 2020
View reviewed changes
cylc/flow/suite_files.py
@@ -339,107 +329,6 @@ def get_contact_file(reg):
get_suite_srv_dir(reg), SuiteFiles.Service.CONTACT)


def get_auth_item(item, reg, owner=None, host=None, content=False):
Copy link
Member

Choose a reason for hiding this comment

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

Goodbye and good riddance!

oliver-sanders
oliver-sanders reviewed May 1, 2020
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Couple of minor comments otherwise looks good.

@datamel datamel force-pushed the authentication_washup_cylc_scan branch from a32cf6e to 200161b Compare May 1, 2020 13:37
@datamel datamel force-pushed the authentication_washup_cylc_scan branch from 902ea78 to 1c065e7 Compare May 4, 2020 12:56
dpmatthews
dpmatthews approved these changes May 5, 2020
View reviewed changes
Copy link
Contributor

@dpmatthews dpmatthews left a comment

Choose a reason for hiding this comment

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

Looks good to me & I've tested that cylc scan now skips cylc 7 suites

@hjoliver
Copy link
Member

hjoliver commented May 5, 2020

Two approvals and an "otherwise looks good" from @oliver-sanders, with the caveat addressed. Merging...

@hjoliver hjoliver merged commit dbd3080 into cylc:master May 5, 2020
@oliver-sanders oliver-sanders mentioned this pull request Jun 15, 2020
Closed
@datamel datamel mentioned this pull request Nov 23, 2020
Merged
7 tasks
@datamel datamel mentioned this pull request Jul 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliver-sanders oliver-sanders oliver-sanders left review comments

@hjoliver hjoliver hjoliver approved these changes

@dpmatthews dpmatthews dpmatthews approved these changes

Assignees

@datamel datamel

Labels
None yet
Projects
None yet
Milestone
cylc-8.0a2
Development

Successfully merging this pull request may close these issues.

KeyError: 'CYLC_SUITE_PUBLISH_PORT' after running suites with Cylc 7 and Cylc 8 authentication: wash up
4 participants
@datamel @hjoliver @dpmatthews @oliver-sanders