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

feat: Add the ability to use automatic subscriber assignment to the subscriber settings #163

Merged
merged 4 commits into from Aug 11, 2020
Merged

feat: Add the ability to use automatic subscriber assignment to the subscriber settings #163

merged 4 commits into from Aug 11, 2020

Conversation

dpcollins-google
Copy link
Collaborator

No description provided.

@dpcollins-google
[feat] Add the ability to use automatic subscriber assignment to the …
c2e8142 
…subscriber settings.

Also disable clirr as it fires on this PR. We are not providing interface stability guarantees.
@dpcollins-google
No-op commit to update github.
0c43e23
@dpcollins-google dpcollins-google added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 15, 2020
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 15, 2020
@anguillanneuf anguillanneuf changed the title [feat] Add the ability to use automatic subscriber assignment to the subscriber settings. feat: Add the ability to use automatic subscriber assignment to the subscriber settings Jul 15, 2020
@anguillanneuf
Copy link
Contributor

Please fix the tests first.

anguillanneuf
anguillanneuf previously requested changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@anguillanneuf anguillanneuf left a comment

Choose a reason for hiding this comment

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

The changes in samples/ looks fine. Please also update .readme-partials.yaml and fix tests.

samples/snippets/pom.xml Outdated
@@ -46,7 +46,7 @@
<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>google-cloud-pubsublite</artifactId>
<version>0.1.7</version>
<version>0.1.9-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't update this to SNAPSHOT as this will get mirrored in https://cloud.google.com/pubsub/lite/docs/reference/libraries
Generally, autosynth will take care of updating version numbers in POM files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Autosynth doesn't seem to be updating this number? At least it didnt in release 1.8 which is confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

It may have something to do with most snippets folders usually don't require version updates / they use libraries bom. @chingor13 is that right? See history: https://github.com/googleapis/java-pubsublite/commits/master/samples/snippets/pom.xml How do we overwrite the update rule?

We never successfully released any version after 0.1.7: https://mvnrepository.com/artifact/com.google.cloud/google-cloud-pubsublite

google-cloud-pubsublite/pom.xml Show resolved Hide resolved
@dpcollins-google
Copy link
Collaborator Author

Please fix the tests first.

The tests are broken due to "[08:14:28][ERROR] Timed out waiting for GitHub to return mergeable status for the pull (PR). GitHub is taking too long to compute whether this PR is mergeable. Please retry the build in a few minutes. You can do this by attaching a kokoro:force-run label on your PR." which sounds like a kokoro issue.

@anguillanneuf anguillanneuf added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 15, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 15, 2020
@dpcollins-google dpcollins-google force-pushed the sub-settings branch 2 times, most recently from afededa to f8039c7 Compare July 15, 2020 20:38
@dpcollins-google
Copy link
Collaborator Author

.readme-partials.yaml updated.

@anguillanneuf
Copy link
Contributor

Samples update should come later, after the library has a new release that picks up the changes. If you want, you can split this into two. Or I can update the samples later.

@codecov
Copy link

codecov bot commented Jul 15, 2020 •
edited

Codecov Report

Merging #163 into master will decrease coverage by 1.09%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #163      +/-   ##
============================================
- Coverage     61.16%   60.07%   -1.10%     
  Complexity      417      417              
============================================
  Files            92       93       +1     
  Lines          2145     2184      +39     
  Branches        181      182       +1     
============================================
  Hits           1312     1312              
- Misses          736      775      +39     
  Partials         97       97
Impacted Files Coverage Δ Complexity Δ
...oud/pubsublite/cloudpubsub/SubscriberSettings.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...loud/pubsublite/internal/wire/AssignerBuilder.java 0.00% <0.00%> (ø) 0.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 3c471ab...214af54. Read the comment docs.

anguillanneuf
anguillanneuf reviewed Jul 15, 2020
View reviewed changes
Copy link
Contributor

@anguillanneuf anguillanneuf left a comment

Choose a reason for hiding this comment

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

I would like @hannahrogers-google or someone else comment on if more tests need to be added before you merge.

@anguillanneuf anguillanneuf self-requested a review July 15, 2020 22:53
@hannahrogers-google
Copy link
Contributor

I would like @hannahrogers-google or someone else comment on if more tests need to be added before you merge.

I am ok with this change - I don't believe we have explicit unit tests for any of the Builders in the lite client library.

anguillanneuf
anguillanneuf reviewed Jul 16, 2020
View reviewed changes
Copy link
Contributor

@anguillanneuf anguillanneuf left a comment

Choose a reason for hiding this comment

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

Cool. I can update samples in a different PR a bit later. Thanks!

@anguillanneuf anguillanneuf self-requested a review July 16, 2020 20:37
@anguillanneuf anguillanneuf dismissed their stale review July 16, 2020 20:38

Changes applied.

lesv
lesv approved these changes Jul 17, 2020
View reviewed changes
Copy link

@lesv lesv left a comment

Choose a reason for hiding this comment

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

This doesn't appear to have a sample - so I'm just approving w/o review.

@anguillanneuf
Copy link
Contributor

@dpcollins-google Does this Do Not Merge label still apply?

@dpcollins-google
Copy link
Collaborator Author

@dpcollins-google Does this Do Not Merge label still apply?

Yes it does. We don't want to merge this until the backend is tested internally and we've updated (/ are updating) the docs.

@anguillanneuf anguillanneuf mentioned this pull request Aug 5, 2020
Merged
@dpcollins-google dpcollins-google removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 11, 2020
@dpcollins-google dpcollins-google merged commit a396f24 into master Aug 11, 2020
@dpcollins-google dpcollins-google deleted the sub-settings branch August 11, 2020 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hannahrogers-google hannahrogers-google hannahrogers-google left review comments

@lesv lesv lesv approved these changes

@anguillanneuf anguillanneuf anguillanneuf approved these changes

@manuelmenzella-google manuelmenzella-google Awaiting requested review from manuelmenzella-google

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.

None yet
6 participants
@dpcollins-google @anguillanneuf @hannahrogers-google @lesv @googlebot @yoshi-kokoro