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

refactor(samples): idtoken-cloudrun => idtoken-serverless #1025

Conversation

grayside
Copy link
Contributor

@grayside grayside commented Aug 6, 2020

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #1019 🦕

I did not see instructions on how to run the sample tests, so going to see what CI has to say.

@grayside grayside requested a review from a team as a code owner August 6, 2020 20:47
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 6, 2020
@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #1025 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1025   +/-   ##
=======================================
  Coverage   91.52%   91.52%           
=======================================
  Files          21       21           
  Lines        4093     4093           
  Branches      488      488           
=======================================
  Hits         3746     3746           
  Misses        347      347           

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 a7e5701...724f62e. Read the comment docs.

@grayside
Copy link
Contributor Author

grayside commented Aug 6, 2020

Not clear how to run the lint/fix locally, and from this result I cannot tell what change is needed:

##[error]  41:18  error  Replace `⏎······`request·${url}·with·target·audience·${targetAudience}`⏎····` with ``request·${url}·with·target·audience·${targetAudience}``  prettier/prettier

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

I see the docs test failure, but I don't see other references within this repo and thus do not know where I need to make other changes:

[404] https://github.com/googleapis/google-auth-library-nodejs/blob/master/samples/idtokens-serverless.js


http://localhost:5758/
  [404] https://github.com/googleapis/google-auth-library-nodejs/blob/master/samples/idtokens-serverless.js
ERROR: Detected 1 broken links. Scanned 51 links in 1.649 seconds.

Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

LGTM, after linting issues are resolved

README.md Show resolved Hide resolved
samples/idtokens-serverless.js Show resolved Hide resolved
@JustinBeckwith
Copy link
Contributor

@grayside for the sample tests, npm run samples-test should get you where you want to go :)
https://github.com/googleapis/google-auth-library-nodejs/blob/master/CONTRIBUTING.md#running-the-tests

If that doesn't work, let us know we'll update the guide!

@grayside
Copy link
Contributor Author

grayside commented Aug 6, 2020

Ah, when I ran npm run fix I saw no output about fixes made and assumed nothing was done, but I see a change!

For npm run samples-test, I don't have access to the project used in this repo and don't see instructions on all the GCP resources that need to be setup. I'll fix the lint and we'll see what happens.

@grayside
Copy link
Contributor Author

grayside commented Aug 6, 2020

Sample test was failing then went to passing. I assume there's some magic afoot...

@bcoe
Copy link
Contributor

bcoe commented Aug 7, 2020

@grayside there's a chicken or the egg issue, where the link checker expects your sample to have already been landed:

  [404] https://github.com/googleapis/google-auth-library-nodejs/blob/master/samples/idtokens-serverless.js

If you add an ignore rule to linkinator.config.json for the above URL, we can then immediately revert that change once we land your work.

@JustinBeckwith
Copy link
Contributor

If you're ok with this to be merged, I can also just hammer merge it.

@grayside
Copy link
Contributor Author

grayside commented Aug 7, 2020

That works for me. Btw, not sure how it works, but shouldn't I be the assignee? I have no rights here.

@JustinBeckwith JustinBeckwith merged commit 11b0cb7 into googleapis:master Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor(samples): Consolidate idtoken samples for serverless
4 participants