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: copy credentials in internal config #1052

Merged
merged 1 commit into from
Jun 26, 2019
Merged

fix: copy credentials in internal config #1052

merged 1 commit into from
Jun 26, 2019

Conversation

kjin
Copy link
Contributor

@kjin kjin commented Jun 24, 2019

Fixes #1048

@kjin kjin requested a review from a team June 24, 2019 23:44
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 24, 2019
@kjin kjin removed the request for review from a team June 24, 2019 23:52
ofrobots
ofrobots reviewed Jun 25, 2019
View reviewed changes
src/index.ts Outdated Show resolved Hide resolved
@kjin kjin force-pushed the copy-credentials branch from ceb8db7 to 0a219f6 Compare June 25, 2019 00:23
@kjin kjin force-pushed the copy-credentials branch from 0a219f6 to 2555b2a Compare June 25, 2019 00:37
@codecov
Copy link

codecov bot commented Jun 25, 2019 •
edited
Loading

Codecov Report

Merging #1052 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1052      +/-   ##
==========================================
+ Coverage   94.85%   94.86%   +0.01%     
==========================================
  Files          96       96              
  Lines        6311     6326      +15     
  Branches      496      496              
==========================================
+ Hits         5986     6001      +15     
  Misses        166      166              
  Partials      159      159
Impacted Files Coverage Δ
src/trace-writer.ts 92.5% <ø> (ø) ⬆️
src/index.ts 89.28% <ø> (ø) ⬆️
test/test-trace-writer.ts 98.45% <ø> (ø) ⬆️
test/test-trace-api.ts 100% <ø> (ø) ⬆️
test/test-config.ts 92.64% <100%> (+2.08%) ⬆️
src/tracing.ts 90% <100%> (ø) ⬆️
test/test-config-priority.ts 95.91% <100%> (ø) ⬆️
test/trace.ts 93.5% <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 178c2a9...2555b2a. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jun 25, 2019

Codecov Report

Merging #1052 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1052      +/-   ##
==========================================
+ Coverage   94.85%   94.86%   +0.01%     
==========================================
  Files          96       96              
  Lines        6311     6326      +15     
  Branches      496      496              
==========================================
+ Hits         5986     6001      +15     
  Misses        166      166              
  Partials      159      159
Impacted Files Coverage Δ
src/trace-writer.ts 92.5% <ø> (ø) ⬆️
src/index.ts 89.28% <ø> (ø) ⬆️
test/test-trace-writer.ts 98.45% <ø> (ø) ⬆️
test/test-trace-api.ts 100% <ø> (ø) ⬆️
test/test-config.ts 92.64% <100%> (+2.08%) ⬆️
src/tracing.ts 90% <100%> (ø) ⬆️
test/test-config-priority.ts 95.91% <100%> (ø) ⬆️
test/trace.ts 93.5% <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 178c2a9...2555b2a. Read the comment docs.

ofrobots
ofrobots reviewed Jun 25, 2019
View reviewed changes
src/index.ts
authOptions: Object.assign({}, mergedConfig, {
projectId: lastOf<string | undefined>(
mergedConfig.projectId,
process.env.GCLOUD_PROJECT
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the auth library already consult this environment variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's unfortunate but I realized that Trace Agent has historically regarded GCLOUD_PROJECT as taking precedence over the projectId config option, while the auth library does the opposite. Maybe it's possible that we change this now, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be semver major :/. Not worth it.

ofrobots
ofrobots approved these changes Jun 26, 2019
View reviewed changes
src/index.ts
authOptions: Object.assign({}, mergedConfig, {
projectId: lastOf<string | undefined>(
mergedConfig.projectId,
process.env.GCLOUD_PROJECT
Copy link
Contributor

Choose a reason for hiding this comment

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

That would be semver major :/. Not worth it.

@kjin kjin merged commit 8930df3 into googleapis:master Jun 26, 2019
@release-please release-please bot mentioned this pull request Jun 30, 2022
Merged
@EitanGayor EitanGayor mentioned this pull request May 31, 2024
Open
@EitanGayor EitanGayor mentioned this pull request Jun 22, 2024
Open
This was referenced Jul 17, 2024
Open
Open
@EitanGayor EitanGayor mentioned this pull request Jul 27, 2024
Open
@EitanGayor EitanGayor mentioned this pull request Aug 22, 2024
Open
This was referenced Sep 6, 2024
Open
Open
Open
@EitanGayor EitanGayor mentioned this pull request Sep 14, 2024
Open
@dazdaz dazdaz mentioned this pull request Sep 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JustinBeckwith JustinBeckwith JustinBeckwith left review comments

@ofrobots ofrobots ofrobots 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.

KeyFilename credentials not copied in internal config
4 participants
@kjin @ofrobots @JustinBeckwith @googlebot