Skip to content

chore: use new GCF env vars in descriptor #436

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
Merged
merged 3 commits into from
Mar 25, 2019
Merged

chore: use new GCF env vars in descriptor #436

merged 3 commits into from
Mar 25, 2019

Conversation

DominicKramer
Copy link
Contributor

The K_SERVICE and GOOGLE_CLOUD_REGION env vars are the
preferred way of getting function name and region information
going forward.

@DominicKramer
chore: use new GCF env vars in descriptor
f790795 
The `K_SERVICE` and `GOOGLE_CLOUD_REGION` env vars are the
preferred way of getting function name and region information
going forward.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 25, 2019
@codecov
Copy link

codecov bot commented Mar 25, 2019

Codecov Report

Merging #436 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
+ Coverage    90.6%   90.63%   +0.02%     
==========================================
  Files          14       14              
  Lines         628      630       +2     
  Branches       32       34       +2     
==========================================
+ Hits          569      571       +2     
  Misses         41       41              
  Partials       18       18
Impacted Files Coverage Δ
src/metadata.ts 100% <100%> (ø) ⬆️

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 7bf4394...f790795. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 25, 2019 •
edited
Loading

Codecov Report

Merging #436 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
+ Coverage    90.6%   90.63%   +0.02%     
==========================================
  Files          14       14              
  Lines         628      630       +2     
  Branches       32       34       +2     
==========================================
+ Hits          569      571       +2     
  Misses         41       41              
  Partials       18       18
Impacted Files Coverage Δ
src/metadata.ts 100% <100%> (ø) ⬆️

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 7bf4394...33167dc. Read the comment docs.

@DominicKramer DominicKramer requested review from JustinBeckwith and a team and removed request for JustinBeckwith March 25, 2019 18:58
callmehiphop
callmehiphop approved these changes Mar 25, 2019
View reviewed changes
Copy link
Contributor

@callmehiphop callmehiphop left a comment

Choose a reason for hiding this comment

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

Is it worth documenting this somewhere in the code? I'm wondering if anyone will get tripped up expecting the old env var to take precedence.

@DominicKramer DominicKramer mentioned this pull request Mar 25, 2019
Closed
JustinBeckwith
JustinBeckwith suggested changes Mar 25, 2019
View reviewed changes
test/metadata.ts Outdated
beforeEach(() => {
process.env.FUNCTION_NAME = FUNCTION_NAME;
process.env.FUNCTION_REGION = FUNCTION_REGION;
delete process.env.K_SERVICE;
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 something like sinon to stub instead? Worried that running the tests will stomp my current session's environment variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sinon looked more complicated than needed, but I've updated the code to use before/after hooks to restore the parts of process.env I am modifying.

@DominicKramer
Copy link
Contributor Author

@callmehiphop I've added code docs.

@DominicKramer DominicKramer merged commit 81138d6 into googleapis:master Mar 25, 2019
@DominicKramer DominicKramer mentioned this pull request Apr 2, 2019
Closed
@jeanregisser jeanregisser mentioned this pull request Nov 28, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JustinBeckwith JustinBeckwith JustinBeckwith approved these changes

@callmehiphop callmehiphop callmehiphop 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.
4 participants
@DominicKramer @JustinBeckwith @callmehiphop @googlebot