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

feat: add access token hub auth strategy #90

Merged
merged 3 commits into from
May 13, 2021
Merged

Conversation

peternhale
Copy link
Contributor

@W-8066452@

@peternhale peternhale requested review from shetzel and mshanemc May 12, 2021 14:40
@codecov-commenter
Copy link

codecov-commenter commented May 12, 2021

Codecov Report

Merging #90 (e9ba2a7) into main (56ab78b) will decrease coverage by 2.44%.
The diff coverage is 13.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
- Coverage   77.57%   75.12%   -2.45%     
==========================================
  Files           6        6              
  Lines         379      394      +15     
  Branches       73       76       +3     
==========================================
+ Hits          294      296       +2     
- Misses         65       76      +11     
- Partials       20       22       +2     
Impacted Files Coverage Δ
src/testSession.ts 89.47% <ø> (ø)
src/hubAuth.ts 26.47% <13.33%> (-2.27%) ⬇️

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 56ab78b...e9ba2a7. Read the comment docs.

Copy link
Contributor

@shetzel shetzel left a comment

Choose a reason for hiding this comment

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

The README.md and SAMPLES.md need to be updated as well, particularly the environment variables sections.

@@ -9,6 +9,8 @@ describe('hubAuth', () => {
it('works with existing defaultdevhub if provided');
it('auths from authurl in env');
it('auths from authurl and sets as default');
it('auths from access token and sets as default');
it('auths from access token');
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we going to ignore these unit tests forever? Even having NUTs for this would be something. Right now there's nothing.

Copy link
Contributor Author

@peternhale peternhale left a comment

Choose a reason for hiding this comment

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

Will add jsdoc

@peternhale peternhale requested a review from shetzel May 13, 2021 17:14
@peternhale peternhale merged commit 296df6e into main May 13, 2021
@peternhale peternhale deleted the phale/W-8066452 branch May 13, 2021 20:02
peternhale added a commit that referenced this pull request May 21, 2021
peternhale added a commit that referenced this pull request May 24, 2021
* Revert "fix: add missed export (#91)"

This reverts commit 86084c8.

* Revert "feat: add access token hub auth strategy (#90)"

This reverts commit 296df6e.
peternhale added a commit that referenced this pull request Jun 10, 2021
peternhale added a commit that referenced this pull request Jun 10, 2021
peternhale added a commit that referenced this pull request Jun 15, 2021
* chore: add unit tests for hutbauth

@W-9270862@

* chore: remove lint error

* Revert "fix: add missed export (#91)"

This reverts commit 86084c8.

* Revert "feat: add access token hub auth strategy (#90)"

This reverts commit 296df6e.

* Revert "feat: add access token hub auth strategy (#90)"

This reverts commit 296df6e.

* chore: mock where I can for unit tests

* chore: get rid of eslint errors

* chore: update some mocking

* chore: arrange tests in an order that works
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants