Skip to content
This repository has been archived by the owner on Dec 20, 2022. It is now read-only.

[patch] Fix authorize #74

Merged
merged 3 commits into from
Aug 17, 2020
Merged

[patch] Fix authorize #74

merged 3 commits into from
Aug 17, 2020

Conversation

ssunorz
Copy link
Contributor

@ssunorz ssunorz commented Aug 12, 2020

Description

description_with_details_and_reasoning

Type of change

  • Bug fix
  • New feature
  • Refactoring (no functional changes, no api changes)
  • Non-code changes (update documentation, pipeline, etc.)

Flags

  • breaks backward compatibility
  • requires a documentation update
  • has untestable code

Related issue/PR

Delete if not applicable

  • Fixes #issue
  • Closes #PR

Checklist

  • Followed the guidelines in the CONTRIBUTING document
  • Added prefix [major]/[minor]/[patch]/[skip] in the PR title
  • Tested and linted the code
  • Commented the code
  • Made corresponding changes to the documentation
  • Confirmed no dropping in test coverage (by Codecov)
  • Passed all pipeline checking
  • Approved by >1 reviewer

Checklist for maintainer

  • Use Squash and merge
  • Double-confirm the merge message has prefix [major]/[minor]/[patch]/[skip]
  • Delete the branch after merge

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2020

Codecov Report

Merging #74 into master will decrease coverage by 0.37%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
- Coverage   94.66%   94.29%   -0.38%     
==========================================
  Files          19       19              
  Lines        1257     1263       +6     
==========================================
+ Hits         1190     1191       +1     
- Misses         45       50       +5     
  Partials       22       22              
Impacted Files Coverage Δ
authorizerd.go 92.30% <100.00%> (+0.19%) ⬆️
policy/daemon.go 91.53% <0.00%> (-1.59%) ⬇️
pubkey/daemon.go 93.10% <0.00%> (-1.38%) ⬇️

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 7c1258f...f8f3c12. Read the comment docs.

return nil
},
}
}(),
Copy link
Contributor

@kyfujisa kyfujisa Aug 14, 2020

Choose a reason for hiding this comment

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

How about adding a test case like the following?

  • disablePolicyd: true false
  • wantResult: p
  • checkFunc: check prov.cache.Get("dummyTok" + "dummyAct" + "dummyRes") is ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed
734d5b5

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I made a mistake.
What I meant to say was the test with disablePolicyd set to false.

Fixed the comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed
f8f3c12

Copy link
Contributor

@kyfujisa kyfujisa left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@WindzCUHK WindzCUHK left a comment

Choose a reason for hiding this comment

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

LGTM

@ssunorz ssunorz merged commit 908920f into master Aug 17, 2020
@ssunorz ssunorz deleted the fix-authorize branch August 17, 2020 03:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants