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

samples: subscription detachment #289

Merged
merged 4 commits into from
Sep 1, 2020
Merged

samples: subscription detachment #289

merged 4 commits into from
Sep 1, 2020

Conversation

anguillanneuf
Copy link
Contributor

@anguillanneuf anguillanneuf commented Jul 15, 2020

Add Java sample for subscription detachment. The test can't be run yet because the feature is still being rolled out.

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

FYI: A DetachSubscriptionRequest is used instead of SubscriptionName.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 15, 2020
@hannahrogers-google hannahrogers-google added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 15, 2020
@anguillanneuf anguillanneuf marked this pull request as ready for review September 1, 2020 18:52
@anguillanneuf anguillanneuf requested a review from a team September 1, 2020 18:52
@anguillanneuf anguillanneuf removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 1, 2020
.addPaths("dead_letter_policy.dead_letter_topic")
// A default of 5 is applied upon successful update.
.addPaths("dead_letter_policy.max_delivery_attempts")
.addPaths("dead_letter_policy")
Copy link

Choose a reason for hiding this comment

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

This, like it's predecessor looks wrong - shouldn't this be a constant defined in the library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Delivery attempt is not a constant defined in the library. In fact, it is best-effort only, and when no dead letter policy is attached, users won't be able to access delivery attempts.

Copy link

Choose a reason for hiding this comment

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

Understood - if it's understood as something important and understood as something special - it should be defined in the library in some way.

@codecov
Copy link

codecov bot commented Sep 1, 2020

Codecov Report

Merging #289 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #289   +/-   ##
=========================================
  Coverage     79.21%   79.21%           
  Complexity      318      318           
=========================================
  Files            21       21           
  Lines          2892     2892           
  Branches        155      155           
=========================================
  Hits           2291     2291           
  Misses          536      536           
  Partials         65       65           

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 0d97689...513c4f4. Read the comment docs.

@anguillanneuf anguillanneuf merged commit 785981a into master Sep 1, 2020
@anguillanneuf anguillanneuf deleted the samples-detach branch September 1, 2020 19:35
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.

4 participants