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.

Sign up for GitHub

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

Jump to bottom

fix!: keyToLegacyUrlsafe is now an async method #496

Merged
merged 21 commits into from
Nov 14, 2019
Merged

fix!: keyToLegacyUrlsafe is now an async method #496

merged 21 commits into from
Nov 14, 2019

Conversation

laljikanjareeya
Copy link
Contributor

@laljikanjareeya laljikanjareeya commented Sep 12, 2019 •
edited
Loading

Fixes #473

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

While calling keyToLegacyUrlsafe function it's not binding {{projectId}} to actual one as it does not have any external/api call.
Added auth.getProjectId to get projectId and change it to callback and promise support as auth.getProjectId function's behavior is same.

BREAKING CHANGE:

  • keyToLegacyUrlsafe is rename to keyToLegacyUrlSafe

  • keyToLegacyUrlSafe is now an async method

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 12, 2019
@codecov
Copy link

codecov bot commented Sep 12, 2019 •
edited
Loading

Codecov Report

Merging #496 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #496      +/-   ##
==========================================
+ Coverage      94%   94.07%   +0.06%     
==========================================
  Files          12       12              
  Lines         918      928      +10     
  Branches      189      192       +3     
==========================================
+ Hits          863      873      +10     
  Misses         44       44              
  Partials       11       11
Impacted Files Coverage Δ
src/index.ts 98.86% <100%> (+0.14%) ⬆️

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 6c8cc74...5e7ac34. Read the comment docs.

@AVaksman AVaksman changed the title (BREAKING CHANGE) feat: assign projectId from GoogleAuth feat!: assign projectId from GoogleAuth Sep 12, 2019
@bcoe bcoe requested a review from AVaksman September 13, 2019 00:53
AVaksman
AVaksman reviewed Sep 13, 2019
View reviewed changes
test/index.ts Outdated Show resolved Hide resolved
@AVaksman
Copy link
Contributor

Besides the extra test
LGTM

@AVaksman AVaksman changed the title feat!: assign projectId from GoogleAuth fix!: assign projectId from GoogleAuth Sep 13, 2019
bcoe
bcoe approved these changes Sep 13, 2019
View reviewed changes
src/index.ts
* All async methods (except for streams) will return a Promise in the event
* that a callback is omitted.
*/
promisifyAll(Datastore, {
Copy link
Contributor

Choose a reason for hiding this comment

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

could we use util.promisify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could we use util.promisify?

@bcoe for consistency promisifyAll is used as currently in all the library we are using the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 works for me, might be worth doing some work on the util module to make sure promisfyAll takes advantage of util.promisify.

@bcoe bcoe changed the title fix!: assign projectId from GoogleAuth fix!: use projectId returned by GoogleAuth Sep 13, 2019
@bcoe bcoe changed the title fix!: use projectId returned by GoogleAuth fix!: assign projectId from GoogleAuth in keyToLegacyUrlsafe Sep 13, 2019
stephenplusplus
stephenplusplus suggested changes Sep 18, 2019
View reviewed changes
src/index.ts Outdated Show resolved Hide resolved
@BenWhitehead
Copy link
Contributor

@stephenplusplus Can you please re-review this after the recent updates?

@bcoe
Copy link
Contributor

bcoe commented Oct 3, 2019

This looks good to me, we should call out what is specifically breaking in the message body of what is landed:

BREAKING CHANGE: keyToLegacyUrlsafe is now an async method

@AVaksman AVaksman changed the title fix!: assign projectId from GoogleAuth in keyToLegacyUrlsafe BREAKING CHANGE!: keyToLegacyUrlsafe is now an async method Oct 4, 2019
@AVaksman AVaksman changed the title BREAKING CHANGE!: keyToLegacyUrlsafe is now an async method fix!: BREAKING CHANGE: keyToLegacyUrlsafe is now an async method Oct 4, 2019
@stephenplusplus stephenplusplus changed the title fix!: BREAKING CHANGE: keyToLegacyUrlsafe is now an async method fix!: keyToLegacyUrlsafe is now an async method Oct 7, 2019
stephenplusplus
stephenplusplus reviewed Oct 7, 2019
View reviewed changes
src/index.ts Outdated Show resolved Hide resolved
stephenplusplus
stephenplusplus suggested changes Oct 7, 2019
View reviewed changes
test/index.ts Outdated Show resolved Hide resolved
test/index.ts Outdated Show resolved Hide resolved
stephenplusplus
stephenplusplus suggested changes Oct 9, 2019
View reviewed changes
src/index.ts Outdated Show resolved Hide resolved
stephenplusplus
stephenplusplus suggested changes Oct 10, 2019
View reviewed changes
src/index.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
stephenplusplus
stephenplusplus reviewed Oct 10, 2019
View reviewed changes
src/index.ts Outdated Show resolved Hide resolved
@bcoe bcoe added the status: blocked Resolving the issue is dependent on other work. label Oct 10, 2019
@bcoe
Copy link
Contributor

bcoe commented Oct 10, 2019

looks like this is ready to land, but let's hold off on landing for a tiny bit, and see if we can batch it up with some breaking changes @AVaksman is working on.

@stephenplusplus
Copy link
Contributor

@bcoe should we still hold off? Feels iffy to keep this floating around 🤷‍♂

@bcoe bcoe removed the status: blocked Resolving the issue is dependent on other work. label Oct 22, 2019
@bcoe bcoe added the status: blocked Resolving the issue is dependent on other work. label Oct 22, 2019
@bcoe bcoe removed the status: blocked Resolving the issue is dependent on other work. label Nov 4, 2019
@crwilcox crwilcox merged commit bbd1ebe into googleapis:master Nov 14, 2019
@release-please release-please bot mentioned this pull request Nov 14, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AVaksman AVaksman AVaksman approved these changes

@bcoe bcoe bcoe approved these changes

@stephenplusplus stephenplusplus stephenplusplus approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

keyToLegacyUrlsafe running on Cloud Functions uses wrong projectId?
8 participants
@laljikanjareeya @AVaksman @BenWhitehead @bcoe @stephenplusplus @crwilcox @googlebot @JustinBeckwith