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

fix: use projectId from CloudStorageConfig #429

Merged
merged 1 commit into from Feb 12, 2021

Conversation

frankyn
Copy link
Member

@frankyn frankyn commented Feb 11, 2021

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:

  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)

Fixes #352 ☕️

@frankyn frankyn requested a review from as a code owner Feb 11, 2021
@google-cla google-cla bot added the cla: yes label Feb 11, 2021
@product-auto-label product-auto-label bot added the api: storage label Feb 11, 2021
@frankyn frankyn requested a review from BenWhitehead Feb 11, 2021
@codecov
Copy link

@codecov codecov bot commented Feb 11, 2021

Codecov Report

Merging #429 (0a02af3) into master (3a3d465) will increase coverage by 0.27%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #429      +/-   ##
============================================
+ Coverage     72.29%   72.57%   +0.27%     
  Complexity      500      500              
============================================
  Files            29       29              
  Lines          1664     1670       +6     
  Branches        277      276       -1     
============================================
+ Hits           1203     1212       +9     
+ Misses          336      335       -1     
+ Partials        125      123       -2     
Impacted Files Coverage Δ Complexity Δ
...le/cloud/storage/contrib/nio/CloudStoragePath.java 78.63% <100.00%> (+2.95%) 52.00 <0.00> (+1.00)
...ud/storage/contrib/nio/testing/FakeStorageRpc.java 70.96% <0.00%> (-0.47%) 47.00% <0.00%> (-1.00%)
...ge/contrib/nio/CloudStorageFileSystemProvider.java 63.23% <0.00%> (ø) 76.00% <0.00%> (ø%)
...storage/contrib/nio/CloudStorageConfiguration.java 88.46% <0.00%> (+2.56%) 14.00% <0.00%> (ø%)

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 3a3d465...0f64570. Read the comment docs.

import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/** Unit tests for {@link CloudStorageFileSystemProvider} late initialization. */
Copy link
Contributor

@BenWhitehead BenWhitehead Feb 11, 2021

Choose a reason for hiding this comment

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

nit: I don't think this comment is true

Copy link
Member Author

@frankyn frankyn Feb 11, 2021

Choose a reason for hiding this comment

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

lol.... copy paste disaster over here!

fileSystem.provider().getProject() == null
? null
: Storage.BlobListOption.userProject(fileSystem.provider().getProject()));
String userProject = fileSystem.config().userProject();
Copy link
Contributor

@BenWhitehead BenWhitehead Feb 11, 2021

Choose a reason for hiding this comment

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

Am I understanding correctly, the bug was reading projectId from the ServiceOptions instead of from the CloudStorageConfig? If so, can we update the commit comment to explain this fact?

Copy link
Member Author

@frankyn frankyn Feb 11, 2021

Choose a reason for hiding this comment

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

Good call, that's much clearer.

@frankyn frankyn force-pushed the fix-user-project-failure branch from f97db4d to 0f64570 Compare Feb 12, 2021
@frankyn frankyn changed the title fix: incorrectly used userProject fix: use projectId from CloudStorageConfig Feb 12, 2021
@frankyn frankyn added the automerge label Feb 12, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit b6ec240 into master Feb 12, 2021
16 checks passed
@gcf-merge-on-green gcf-merge-on-green bot deleted the fix-user-project-failure branch Feb 12, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge label Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants