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

Have authorise handle key errors when getting group names from ids #272

Merged
merged 2 commits into from
Nov 9, 2021

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Nov 3, 2021

This is a small change with no associated Issue.

Description of issue:

I got KeyError from cylc.uiserver.authorise.parse_group_ids because my profile is messed up and keys I wanted were not available.

Description of response

  • Added an exception for this case, and created logging for it too.
  • Additionally discovered that the logging in authorise.py wasn't working, so modified the app.py to pass the logger object in.

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.
  • No dependency changes.

@wxtim wxtim added the bug Something isn't working label Nov 3, 2021
@wxtim wxtim added this to the cylc-uiserver 1.0.0 milestone Nov 3, 2021
@wxtim wxtim self-assigned this Nov 3, 2021
@wxtim wxtim force-pushed the fix.get_group_handles_key_errors branch from 9332242 to 7f2507f Compare November 3, 2021 16:36
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.

Looks good, haven't manually tested, MyPy is unhappy for some reason.

cylc/uiserver/authorise.py Outdated Show resolved Hide resolved
@wxtim wxtim force-pushed the fix.get_group_handles_key_errors branch from c683dee to 13a1218 Compare November 4, 2021 09:15
@codecov-commenter
Copy link

Codecov Report

Merging #272 (c683dee) into master (fea9786) will decrease coverage by 0.06%.
The diff coverage is 84.21%.

❗ Current head c683dee differs from pull request most recent head 13a1218. Consider uploading reports for the commit 13a1218 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #272      +/-   ##
==========================================
- Coverage   77.62%   77.55%   -0.07%     
==========================================
  Files          12       12              
  Lines        1001     1007       +6     
  Branches      173      175       +2     
==========================================
+ Hits          777      781       +4     
  Misses        195      195              
- Partials       29       31       +2     
Impacted Files Coverage Δ
cylc/uiserver/app.py 86.44% <ø> (ø)
cylc/uiserver/authorise.py 84.84% <84.21%> (-0.49%) ⬇️

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 fea9786...13a1218. Read the comment docs.

@wxtim wxtim force-pushed the fix.get_group_handles_key_errors branch from 13a1218 to 7097548 Compare November 4, 2021 09:18
Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

I have currently working groups so I can not manually check this. I have read the code and am happy with the logic. Thanks @wxtim, and good catch on the issue!

@oliver-sanders
Copy link
Member

@datamel happy to merge?

@wxtim
Copy link
Member Author

wxtim commented Nov 8, 2021

Worth saying that it seems to work for Dave.

@datamel datamel merged commit d9f2d9e into master Nov 9, 2021
@oliver-sanders oliver-sanders deleted the fix.get_group_handles_key_errors branch November 9, 2021 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants