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

feat: expose all the methods of notification #141

Merged
merged 6 commits into from Jun 25, 2020

Conversation

athakor
Copy link
Contributor

@athakor athakor commented Feb 20, 2020

Fixes #138

This PR exposing all the methods of notifications, also contain the unit test as well as system test of each method.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 20, 2020
@codecov
Copy link

codecov bot commented Feb 20, 2020

Codecov Report

Merging #141 into master will increase coverage by 0.48%.
The diff coverage is 80.55%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #141      +/-   ##
============================================
+ Coverage     63.13%   63.61%   +0.48%     
- Complexity      601      632      +31     
============================================
  Files            32       33       +1     
  Lines          5078     5222     +144     
  Branches        481      498      +17     
============================================
+ Hits           3206     3322     +116     
- Misses         1708     1730      +22     
- Partials        164      170       +6     
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/com/google/cloud/storage/Storage.java 80.90% <ø> (ø) 0.00 <0.00> (ø)
...om/google/cloud/storage/spi/v1/HttpStorageRpc.java 1.50% <0.00%> (-0.02%) 1.00 <0.00> (ø)
...ogle/cloud/storage/spi/v1/HttpStorageRpcSpans.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...va/com/google/cloud/storage/spi/v1/StorageRpc.java 64.86% <ø> (ø) 0.00 <0.00> (ø)
...ain/java/com/google/cloud/storage/StorageImpl.java 80.24% <72.41%> (-0.29%) 135.00 <4.00> (+4.00) ⬇️
...ava/com/google/cloud/storage/NotificationInfo.java 90.38% <90.38%> (ø) 26.00 <26.00> (?)
...ogle/cloud/storage/testing/StorageRpcTestBase.java 98.00% <100.00%> (+0.04%) 49.00 <1.00> (+1.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 34d1633...300c604. Read the comment docs.

Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Please don't rely on PubSub in Notification support.

@@ -80,6 +80,14 @@
<groupId>com.google.protobuf</groupId>
<artifactId>protobuf-java-util</artifactId>
</dependency>
<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>google-cloud-pubsub</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

Don't rely on pubsub in this case please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@frankyn
Copy link
Member

frankyn commented Feb 26, 2020

There's a linkage issue:

SEVERE: Newly introduced problem:
(google-cloud-nio-0.120.0-alpha.jar) com.google.cloud.storage.contrib.nio.testing.FakeStorageRpc's method getNotification(String arg1, String arg2) is not implemented in the class
  referenced from com.google.cloud.storage.spi.v1.StorageRpc (google-cloud-storage-1.104.0.jar)

@athakor
Copy link
Contributor Author

athakor commented Feb 27, 2020

@frankyn here we cant do anything to fix this issue,we can fix it in java-storage-nio. I have already raised the issue over there you can see here storage-nio-38.

@athakor
Copy link
Contributor Author

athakor commented Mar 31, 2020

@frankyn here, how we can move ahead with above situation?

@frankyn
Copy link
Member

frankyn commented Apr 2, 2020

Discussing this now in #179, looks like we've been making interface breaking changers and not bumping java major version. Apologies for the delay @athakor

@athakor
Copy link
Contributor Author

athakor commented Apr 16, 2020

@frankyn Updated branch as per 241 PTAL when you get chance.

pom.xml Outdated Show resolved Hide resolved
@athakor athakor added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 17, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 17, 2020
@frankyn
Copy link
Member

frankyn commented Jun 17, 2020

@athakor could you merge master and resolve conflicts?

@athakor
Copy link
Contributor Author

athakor commented Jun 18, 2020

@athakor could you merge master and resolve conflicts?

@frankyn done. still there's a linkage issue.

@frankyn
Copy link
Member

frankyn commented Jun 18, 2020

Thanks, we need one more release of the libraries-bom. Should be coming soon and wanted to prep to have your pr ready.

@athakor
Copy link
Contributor Author

athakor commented Jun 18, 2020

@frankyn thanks for your quick response.

@frankyn frankyn added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 23, 2020
@frankyn frankyn closed this Jun 23, 2020
@frankyn frankyn reopened this Jun 23, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 23, 2020
@frankyn
Copy link
Member

frankyn commented Jun 23, 2020

whoops, didn't mean to close. libraries-bom released and rerunning tests.

@athakor athakor added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 25, 2020
@yoshi-kokoro yoshi-kokoro removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 25, 2020
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

LGTM

@frankyn frankyn merged commit 8dfc0cb into googleapis:master Jun 25, 2020
frankyn added a commit that referenced this pull request Jun 25, 2020
frankyn added a commit that referenced this pull request Jun 25, 2020
@release-please release-please bot mentioned this pull request Jun 25, 2020
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.

Expose the methods of Notifications
4 participants