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

make reader compatible with google-cloud-logging v3 releases #27

Merged
merged 9 commits into from
Nov 1, 2024

Conversation

jchapian
Copy link
Contributor

@jchapian jchapian commented Oct 31, 2024

Overview
This PR adds support for google-cloud-logging >= 3.0... This will transitively require google-api-core>=2.0. The primary changes are as follows:

  • logging_client.list_entries no longer has a projects argument... It now accepts a list of resources. Internally if resources is not provided, it will default to [f'projects/{self.project_id}'] which is the project_id associated with the client. We make use of this argument, so behavior should be 1:1 to what we are doing now (for multi and single project cases.)
  • list_entries now pages for you in both the http logging_api and the gapic logging_api.

@jchapian jchapian marked this pull request as ready for review October 31, 2024 22:26
@jchapian jchapian requested a review from mjschultz October 31, 2024 22:26
@@ -25,7 +25,7 @@ classifiers =
packages = find:
python_requires = >=3.6
install_requires =
google-cloud-logging < 2.0
google-cloud-logging < 4.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Due to the major library upgrade, I think we should bump the version to 3.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is backwards compatible with 1.x and 2.x 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But, I'm fine with that... I'm not familiar with how you all have been doing versioning. Was thinking major bumps were reserved for breaking changes

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to be conservative and we should bump the major version since I'm not sure what dependencies this upgrade entails and the impact it would have on consuming project policies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jchapian jchapian requested a review from mjschultz October 31, 2024 23:50
@mcneo
Copy link
Contributor

mcneo commented Nov 1, 2024

lgtm

jchapian and others added 3 commits October 31, 2024 20:18
@mjschultz mjschultz merged commit cda4808 into main Nov 1, 2024
10 checks passed
@mjschultz mjschultz deleted the feature/logging-client-compatibility branch November 1, 2024 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants