-
Notifications
You must be signed in to change notification settings - Fork 80
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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.
google-cloud-storage/pom.xml
Outdated
@@ -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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There's a linkage issue:
|
@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. |
@frankyn here, how we can move ahead with above situation? |
@athakor could you merge master and resolve conflicts? |
…ix the builds checks
Thanks, we need one more release of the libraries-bom. Should be coming soon and wanted to prep to have your pr ready. |
@frankyn thanks for your quick response. |
whoops, didn't mean to close. libraries-bom released and rerunning tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #138
This PR exposing all the methods of notifications, also contain the unit test as well as system test of each method.