-
Notifications
You must be signed in to change notification settings - Fork 39
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
feat: implement listLogs API and provide sample snippet #602
Conversation
Here is the summary of changes. You are about to add 3 region tags.
You are about to delete 1 region tag.
This comment is generated by snippet-bot.
|
google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
Outdated
Show resolved
Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
Show resolved
Hide resolved
google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java
Outdated
Show resolved
Hide resolved
google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java
Show resolved
Hide resolved
samples/snippets/src/main/java/com/example/logging/ListLogs.java
Outdated
Show resolved
Hide resolved
3b70732
to
3470716
Compare
The XML file was modified to narrow the opted out scope. The default method implementations were provided.
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.
Feature looks fine to me (aside from the method name on the Rpc interface/implementation).
New samples need to follow the rubric/format set by the Java samples working group: https://github.com/GoogleCloudPlatform/java-docs-samples/blob/master/SAMPLE_FORMAT.md. If that is significant work, consider breaking apart the feature PR and the sample PR
google-cloud-logging/src/main/java/com/google/cloud/logging/spi/v2/LoggingRpc.java
Outdated
Show resolved
Hide resolved
samples/snippets/src/main/java/com/example/logging/ListLogs.java
Outdated
Show resolved
Hide resolved
google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java
Show resolved
Hide resolved
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.
Great work!
google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
Show resolved
Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
Show resolved
Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
Show resolved
Hide resolved
google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java
Show resolved
Hide resolved
google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java
Outdated
Show resolved
Hide resolved
@chingor13 i did not find the reason why the snippet-bot fails. the region tags in ListLogs.java and ListLogEntries.java match and I am unaware of other tests that the bot may perform. maybe it is due to the split of the snippets. |
The way snippets work on Cloudsite is the reference the snippet by GitHub repo, file path, and snippet name. By moving the snippet to a different file, it will break the sample usage. Moving a snippet to a new file or repo involves 3 steps:
|
379aca9
to
df90c9f
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.
nit on license header in new file
Otherwise make sure we're not breaking the snippets
samples/snippets/src/main/java/com/example/logging/ListLogEntries.java
Outdated
Show resolved
Hide resolved
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.
We do not bump copyrights.
Please ping me if you want details.
c0aa966
to
df90c9f
Compare
@lesv the copyright year change for existing files are rolled back. the copyright year is adjusted for a new file only. |
Add listLogs API support to hand-written layer of google-cloud-logging. Add unit testing for the new listLogs API. Fixes #593
Add a sample snippet to demonstrate use of listLogs API. Refactor ListLogs to include snippets for listLogs and listLogEntries. Format all snippets. Fixes #358.
Because of JDK 1.7 it is impossible to provide default implementation for new interface methods. File with exclusions is added instead. The file should be removed once JDK version is upgraded.
Fix printed string in LogEntryWriteHttpRequest.createLogEntryRequest(). Fix loops to wait for any data in STDOUT. Add test for listLogs snippet.
Update testListLogNames() signature to throw exceptions
Test ListLogs.printLogNames vs audit logs to save time. Restore retrieval of log entries in the wait loop to ensure printing to STDOUT
Provide method level exception configuration in clirr-ignored-differences. Implement default methods for new methods in Logging and LoggingRpc interfaces. Following guidelines, remove serialVersionUID from LogNamePageFetcher.
Make more verbose naming for methods. Refactor testing after renaming interface method(s). Split ListLogs sample into two: ListLogEntries and ListLogs.
update the list log entries filter to bring results only for the last hour.
adding empty region tag logging_list_log_entries to ListLogs.java
100e578
to
a67a83c
Compare
Woa - this seems wrong. Where are you hearing about this process -- I'd like to fix it. |
Note - my LGTM was only for the copyrights. Let me take another look. |
google-cloud-logging/src/main/java/com/google/cloud/logging/Logging.java
Show resolved
Hide resolved
In thinking about it, they are trying to create an interface with default
methods here. (a Java 8 feature). Since *logging* will never be created
directly, you'll never execute this code. I believe I closed the comment
before I LGTM'd the review. Sorry it was sent.
…On Thu, Aug 12, 2021 at 8:00 AM minherz ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
google-cloud-logging/src/main/java/com/google/cloud/logging/Logging.java
<#602 (comment)>
:
> + * <p>Example of listing log names, specifying the page size.
+ *
+ * ***@***.***
+ * Page<Log> logNames = logging.listLogs(ListOption.pageSize(100));
+ * Iterator<Log> logIterator = logNames.iterateAll().iterator();
+ * while (logIterator.hasNext()) {
+ * String logName = logIterator.next();
+ * // do something with the log name
+ * }
+ * }</pre>
+ *
+ * @throws LoggingException upon failure
+ */
+ default Page<String> listLogs(ListOption... options) {
+ throw new UnsupportedOperationException(
+ "method listLogs() does not have default implementation");
+ }
@lesv <https://github.com/lesv> can you ref me to guideline about
providing (or not) examples in javadoc?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#602 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIVLDSSDHYK5MZZ5EZR44TT4QD3VANCNFSM5BIKN6MA>
.
|
🤖 I have created a release \*beep\* \*boop\* --- ## [3.1.0](https://github.com/googleapis/java-logging/compare/v3.0.1...v3.1.0) (2021-08-24) ### Features * implement listLogs API and provide sample snippet ([#602](https://github.com/googleapis/java-logging/issues/602)) ([9359569](https://github.com/googleapis/java-logging/commit/935956944200d978738d86ae4adff6107532531c)) ### Dependencies * update dependency com.google.cloud:google-cloud-shared-dependencies to v2.1.0 ([#636](https://github.com/googleapis/java-logging/issues/636)) ([fb9ac95](https://github.com/googleapis/java-logging/commit/fb9ac954293f5a281024e122d06e8487cb3a62c1)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Implements listLogs interface for com.google.cloud.logging.Logging interface.
Implements unit testing for the listLogs interface.
Adds sample snippet for listLogs interface.
Refactors com.example.logging.ListLogs to incorporate listLogs snippet into the class.
Fixes #593
Fixes #358