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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

samples: update TODO section #1218

Merged
merged 3 commits into from
Jul 26, 2021
Merged

samples: update TODO section #1218

merged 3 commits into from
Jul 26, 2021

Conversation

averikitsch
Copy link
Contributor

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 #<issue_number_goes_here> 馃

@averikitsch averikitsch requested review from a team as code owners July 23, 2021 18:27
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 23, 2021
@averikitsch
Copy link
Contributor Author

.origin will strip the GCF url of the actual function route. I have tested with Cloud Run and it works either way.

@averikitsch averikitsch added the automerge Merge the pull request once unit tests and other checks pass. label Jul 23, 2021
@gcf-merge-on-green
Copy link
Contributor

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jul 24, 2021
@averikitsch
Copy link
Contributor Author

@googleapis/yoshi-nodejs PTAL

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.

One issue with the snippet we should discuss - also, please use docs: or samples: - when you use fix:, it signals to the release tooling that this should cause a release. This PR doesn't have any material impact on the library code, so we wouldn't want to do that.

@@ -29,14 +29,15 @@ function main(
* TODO(developer): Uncomment these variables before running the sample.
*/
// const url = 'https://TARGET_URL';
// const targetAudience = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

As written, this code would fail because we're using a const instead of let, and trying to assign down on line 40. Would another option here be moving const {URL} = require('url'); with the other requires, and always doing the assignment of const targetAudience = new URL(url) on line 32?

@averikitsch averikitsch changed the title fix: update TODO section samples: update TODO section Jul 26, 2021
@averikitsch averikitsch merged commit 804a430 into master Jul 26, 2021
@averikitsch averikitsch deleted the sample-update branch July 26, 2021 20:33
xil222 pushed a commit to xil222/google-auth-library-nodejs that referenced this pull request Jul 29, 2021
* fix: update TODO section

* remove origin

* Update idtokens-serverless.js
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.

None yet

2 participants