-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Broaden google-cloud-core dependency #2094
Broaden google-cloud-core dependency #2094
Conversation
Hi @ptoman-pa. Thanks for your PR. I'm waiting for a feast-dev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Codecov Report
@@ Coverage Diff @@
## master #2094 +/- ##
==========================================
- Coverage 83.45% 73.95% -9.51%
==========================================
Files 100 98 -2
Lines 8088 8020 -68
==========================================
- Hits 6750 5931 -819
- Misses 1338 2089 +751
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
hey @ptoman-pa, thanks for your PR! we require PRs to be signed; would you mind signing your commit and rebasing? thanks! |
e71d8f7
to
2c36e3b
Compare
@felixwang9817 Thank you for the polite reminder! Signed. |
hey @ptoman-pa, I don't think the signing worked properly. if you look at your commit, you'll see it doesn't have the "signed off by" note that other commits have - could you try signing again with |
ddde51e
to
d6cca6a
Compare
Done. Also added a note about the project wanting signoffs rather than GPG, but we can remove that or break it into a separate PR depending on your preferences for repo management. |
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
thanks for adding that note to our docs @ptoman-pa! it looks like the first of two commits is signed off properly, but the second isn't - would you mind fixing that? apologies for all the back and forth! |
Limiting google-cloud-core to 1.4.* seems tighter than it needs to be. The version 1.7.2 (which is the latest before 2.0.0) passes tests and makes feast more interoperable as a library with other systems. Signed-off-by: Pamela Toman <[email protected]>
Signed-off-by: Pamela Toman <[email protected]>
d6cca6a
to
2aa2256
Compare
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: felixwang9817, ptoman-pa The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Broadens dependency on google-cloud-core. The limitation to 1.4.* seems tighter than it needs to be. The version 1.7.2 (which is the latest before 2.0.0) passes unit tests and makes feast more interoperable as a library with other systems.
Which issue(s) this PR fixes:
Relates to #928
Does this PR introduce a user-facing change?: