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

Pass authenticated user to cylc-flow #288

Merged
merged 4 commits into from
Jan 25, 2022

Conversation

datamel
Copy link
Contributor

@datamel datamel commented Nov 17, 2021

This is necessary for logging which authenticated user executes the command on the UI.
Sibling pr: cylc/cylc-flow#4522
These changes close #224 and cylc/cylc-flow#4294

  • 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).
  • Does not need tests - can not test this currently due to no multi user testing in the automated test battery.
  • No change log entry required -invisible to users.
  • No documentation update required.
  • No dependency changes.

@codecov-commenter
Copy link

Codecov Report

Merging #288 (62f1f22) into master (37cc11a) will decrease coverage by 0.07%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #288      +/-   ##
==========================================
- Coverage   77.48%   77.40%   -0.08%     
==========================================
  Files          12       12              
  Lines        1008     1009       +1     
  Branches      176      176              
==========================================
  Hits          781      781              
- Misses        196      197       +1     
  Partials       31       31              
Impacted Files Coverage Δ
cylc/uiserver/resolvers.py 32.25% <0.00%> (-0.36%) ⬇️
cylc/uiserver/workflows_mgr.py 90.90% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37cc11a...62f1f22. Read the comment docs.

@datamel datamel force-pushed the pass-username-to-cylc-flow branch from 62f1f22 to 436dd86 Compare December 15, 2021 14:37
@datamel datamel requested a review from wxtim December 16, 2021 11:43
wxtim
wxtim previously approved these changes Dec 16, 2021
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

  • Code makes sense
  • Demoed
  • Passing tests

@wxtim wxtim self-requested a review December 16, 2021 14:23
@wxtim wxtim dismissed their stale review December 16, 2021 14:23

Pressed wrong button

@datamel
Copy link
Contributor Author

datamel commented Dec 16, 2021

Note test failure expected as it requires sibling cylc-flow to be checked out.

  • Code makes sense
  • Demoed
  • Passing tests

Ah, tests will only pass if checked out with sibling on cylc-flow.

@wxtim
Copy link
Member

wxtim commented Dec 17, 2021

I can confirm that tests pass if Sibling PR is checked out. I will leave you to merge this and the sibling at the same time so as not to break master.

@datamel datamel force-pushed the pass-username-to-cylc-flow branch from 7dfc98b to 8bbf0ab Compare January 19, 2022 11:17
@datamel datamel force-pushed the pass-username-to-cylc-flow branch from 8bbf0ab to 27f9d79 Compare January 19, 2022 11:18
@@ -224,29 +227,3 @@ async def service(self, info, *m_args):
self.workflows_mgr,
log=self.log
)

async def nodes_mutator(self, info, *m_args):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dwsutherland, any chance you could double check that this is unused code. I couldn't get any breakpoints stopping here, can't find any code calls. Both myself and @oliver-sanders think it is not used and should be removed? Also, removed its cylc-flow partner in https://github.com/cylc/cylc-flow/pull/4522/files.

Copy link
Member

@dwsutherland dwsutherland Feb 1, 2022

Choose a reason for hiding this comment

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

It was in use, until:
cylc/cylc-flow#3469
specifically:
https://github.com/cylc/cylc-flow/pull/3469/files#diff-97f4d62be41218e2d70a203edfca6d44d7c0046b6908cefa8e3af7c02cd31920
(it was called from the schema which is inherited here)

And the idea behind it was that mutations being multi-workflow could be given node ids from different workflows, and then these ids would be parsed and mutations sent to the appropriate workflow..

The fact that we moved away from this, means we are happy with not using this functionality... i.e. a node mutation will always be associated with only one workflow and/or the workflow parsing happens somewhere else.

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.

(tests pass locally with the cylc-flow branch checked out)

@MetRonnie MetRonnie closed this Jan 25, 2022
@MetRonnie MetRonnie reopened this Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mutations: log the authenticated user
6 participants