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 destination property into LogEntry #720

Merged
merged 11 commits into from Oct 24, 2021
Merged

feat: Add destination property into LogEntry #720

merged 11 commits into from Oct 24, 2021

Conversation

losalex
Copy link
Contributor

@losalex losalex commented Oct 21, 2021

Added ResourceName which will provide destination log resource name customization for LogEntry

Fixes #719 ☕️

@losalex
Add ResourceName class which will provide destination log resource na…
5a86394 
…me customization and integrate it with LogEntry
@losalex losalex requested a review from minherz October 21, 2021 02:52
@product-auto-label product-auto-label bot added the api: logging Issues related to the googleapis/java-logging API. label Oct 21, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 21, 2021
minherz
minherz requested changes Oct 21, 2021
View reviewed changes
google-cloud-logging/src/main/java/com/google/cloud/logging/LogEntry.java Outdated Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/ResourceName.java Outdated Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/ResourceName.java Outdated Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/ResourceName.java Outdated Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/ResourceName.java Outdated Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/ResourceName.java Outdated Show resolved Hide resolved
google-cloud-logging/src/test/java/com/google/cloud/logging/LogEntryTest.java Outdated Show resolved Hide resolved
google-cloud-logging/src/test/java/com/google/cloud/logging/LogEntryTest.java Outdated Show resolved Hide resolved
google-cloud-logging/src/test/java/com/google/cloud/logging/LogEntryTest.java Outdated Show resolved Hide resolved
google-cloud-logging/src/test/java/com/google/cloud/logging/LogEntryTest.java Outdated Show resolved Hide resolved
@losalex losalex changed the title Add ResourceName class and integrate it with LogEntry feat: Add ResourceName class and integrate it with LogEntry Oct 22, 2021
minherz
minherz requested changes Oct 22, 2021
View reviewed changes
Copy link
Contributor

@minherz minherz left a comment

Choose a reason for hiding this comment

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

Please, address a case when the log name destination is lost when parsing it from protobuf.
The code in unit test should be aligned with Java guidelines (constants naming convention and multiple use of the same string literals).
Also consider other non-blocking comments.

google-cloud-logging/src/main/java/com/google/cloud/logging/LogDestinationName.java Outdated Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/LogDestinationName.java Outdated Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/LogEntry.java Outdated Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/LogEntry.java Outdated Show resolved Hide resolved
google-cloud-logging/src/test/java/com/google/cloud/logging/LogEntryTest.java Outdated Show resolved Hide resolved
google-cloud-logging/src/test/java/com/google/cloud/logging/LogEntryTest.java Outdated Show resolved Hide resolved
google-cloud-logging/src/test/java/com/google/cloud/logging/LogEntryTest.java Outdated Show resolved Hide resolved
google-cloud-logging/src/test/java/com/google/cloud/logging/LogEntryTest.java Outdated Show resolved Hide resolved
google-cloud-logging/src/test/java/com/google/cloud/logging/LogEntryTest.java Outdated Show resolved Hide resolved
@losalex losalex marked this pull request as ready for review October 22, 2021 21:26
@losalex losalex requested review from a team as code owners October 22, 2021 21:26
minherz
minherz approved these changes Oct 24, 2021
View reviewed changes
Copy link
Contributor

@minherz minherz left a comment •
edited

Choose a reason for hiding this comment

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

Good work! Couple of things to do before you merge:

  • Run formatting again to make sure PR pass lint check
  • Rebase to the latest version of the main branch (ping me if you have concerns about the way to do it)

Than you

google-cloud-logging/src/main/java/com/google/cloud/logging/LogDestinationName.java Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/LogEntry.java Show resolved Hide resolved
google-cloud-logging/src/test/java/com/google/cloud/logging/LogEntryTest.java Show resolved Hide resolved
@minherz minherz changed the title feat: Add ResourceName class and integrate it with LogEntry feat: Add destination property into LogEntry Oct 24, 2021
@googleapis googleapis deleted a comment from losalex Oct 24, 2021
@losalex losalex merged commit 43ea0b4 into main Oct 24, 2021
@losalex losalex deleted the losalex/719 branch October 24, 2021 20:43
@losalex losalex mentioned this pull request Oct 25, 2021
Closed
@losalex losalex assigned losalex and unassigned simonz130 Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@minherz minherz minherz approved these changes

Assignees

@losalex losalex

Labels
api: logging Issues related to the googleapis/java-logging API. 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
3 participants
@losalex @minherz @simonz130