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

flow-cfg-update.authentication #3845

Merged
merged 2 commits into from
Oct 1, 2020
Merged

flow-cfg-update.authentication #3845

merged 2 commits into from
Oct 1, 2020

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Sep 30, 2020

Refers to Cylc8 Config Proposal agreed at CylcCon Wellington.

Do we want to attach a follow on to carry out more heavy-duty removal of this functionality?

@wxtim wxtim added small question Flag this as a question for the next Cylc project meeting. labels Sep 30, 2020
@wxtim wxtim self-assigned this Sep 30, 2020
@wxtim wxtim added this to the cylc-8.0a4 milestone Sep 30, 2020
@wxtim wxtim force-pushed the flow-cfg-update.authentication branch from 265891b to fbad477 Compare September 30, 2020 14:08
MetRonnie
MetRonnie approved these changes Oct 1, 2020
View reviewed changes
Copy link
Member

@MetRonnie MetRonnie 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 as far as I can tell

oliver-sanders
oliver-sanders reviewed Oct 1, 2020
View reviewed changes
cylc/flow/network/server.py
@@ -15,7 +15,7 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
"""Server for suite runtime API."""

import getpass
import getpass # noqa: F401
Copy link
Member

Choose a reason for hiding this comment

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

What's this F401 for?

Copy link
Member Author

@wxtim wxtim Oct 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

getpass is not used overtly, but causes failures if it's removed. Detailed cause not investigated.

Copy link
Member

Choose a reason for hiding this comment

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

What failures occurred? I just tried removing this line and didn't see any problems on running a flow.

oliver-sanders
oliver-sanders reviewed Oct 1, 2020
View reviewed changes
cylc/flow/cfgspec/suite.py
@@ -21,7 +21,6 @@

from cylc.flow import LOG
from cylc.flow.parsec.exceptions import UpgradeError
from cylc.flow.network.authorisation import Priv
Copy link
Member

Choose a reason for hiding this comment

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

Can remove the Priv class (as this PR effectively removes authorisation) and kill the if usr_priv_level < req_priv_level: check.

@oliver-sanders
Copy link
Member 

2020-10-01T10:25:53Z ERROR - pb_entire_workflow() got an unexpected keyword
	argument 'user'
	Traceback (most recent call last):
	  File "/Users/oliver/cylc-flow/cylc/flow/network/server.py", line 245, in _receiver
	    response = method(**args)
	TypeError: pb_entire_workflow() got an unexpected keyword argument 'user'

@wxtim wxtim force-pushed the flow-cfg-update.authentication branch from 987a21f to d7ba6a4 Compare October 1, 2020 12:43
@oliver-sanders oliver-sanders removed the question Flag this as a question for the next Cylc project meeting. label Oct 1, 2020
@oliver-sanders oliver-sanders merged commit f75322b into cylc:master Oct 1, 2020
@oliver-sanders
Copy link
Member

@wxtim BTW this will require a sibling PR in cylc-doc

@oliver-sanders oliver-sanders modified the milestones: cylc-8.0a4, cylc-8.0a3 Oct 1, 2020
@MetRonnie MetRonnie mentioned this pull request Oct 6, 2020
Closed
@oliver-sanders oliver-sanders mentioned this pull request Oct 14, 2020
Merged
@hjoliver hjoliver modified the milestones: cylc-8.0a3, cylc-8.0b0 Feb 25, 2021
@MetRonnie MetRonnie mentioned this pull request Apr 27, 2021
Closed
@wxtim wxtim deleted the flow-cfg-update.authentication branch March 22, 2022 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hjoliver hjoliver hjoliver left review comments

@MetRonnie MetRonnie MetRonnie approved these changes

@oliver-sanders oliver-sanders oliver-sanders approved these changes

Assignees

@wxtim wxtim

Labels
small
Projects
None yet
Milestone
cylc-8.0b0
Development

Successfully merging this pull request may close these issues.
4 participants
@wxtim @oliver-sanders @hjoliver @MetRonnie