-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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(dataplex): add code samples for search Entries #9609
base: main
Are you sure you want to change the base?
Conversation
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
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.
good coding styles.
please refactor the code to address the comments.
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.
IF you want to demonstrate pagination, please refactor the code to either
- return all results from all pages for the given number of pages
- implement a smart iterator that "knows" to pull additional results when reaches the end of the page
either of them should be placed in the method that shows the implementation.
IF you want to demonstrate how to pull query results ONLY, remove paging from the picture and increase the size of the page to, say, 100 entries.
List<Entry> entries = | ||
searchEntriesResponse.getResultsList().stream() | ||
// Extract Entries nested inside search results | ||
.map(SearchEntriesResult::getDataplexEntry) | ||
.collect(Collectors.toList()); |
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.
please place this code into the code sample. if this code is not intended to demonstrate anything then remove this code. the main()
method should be a simple setup and a call to the code samples. additional transformations in main()
are confusing because it is unclear whether it is part of the code sample or not.
// There are 2 ways to limit the scope of the search: | ||
// 1) Set the "scope" field, with either "projects/<>" or "organizations/<>", | ||
// which will limit the search to resources residing in given project / organization. | ||
// 2) Leave "scope" field blank. In that case scope will be limited to organization in | ||
// which project from "name" field is located. | ||
// Please note that field "name" must always be filled, as the request itself will be | ||
// attributed to that. |
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.
such large comments in code samples point to a problem. either create two code samples to demonstrate different principles or remove this comment.
if you decide to use two code samples, please follow a code sample per class principle.
expectedEntry = | ||
String.format("locations/%s/entryGroups/%s/entries/%s", LOCATION, entryGroupId, entryId); |
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.
need refactoring: move this to be a constant in the class declaration like LOCATION
or ID
@BeforeClass | ||
public static void checkRequirements() { | ||
requireProjectIdEnvVar(); | ||
} |
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.
need refactoring: please combine all initial setup into a single method
CreateEntryGroup.createEntryGroup(PROJECT_ID, LOCATION, entryGroupId); | ||
// Create Entry that will be used in tests | ||
CreateEntry.createEntry(PROJECT_ID, LOCATION, entryGroupId, entryId); | ||
// Wait 30 seconds to allow Entry to propagate to Search |
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.
delete this comment. the code is self-explanatory
} | ||
|
||
@AfterClass | ||
// Clean-up code that will be executed after all tests |
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.
delete this comment. the code is self-explanatory
} | ||
|
||
@BeforeClass | ||
// Set-up code that will be executed before all tests |
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.
delete this comment. the code is self-explanatory
@AfterClass | ||
// Clean-up code that will be executed after all tests | ||
public static void tearDown() throws Exception { | ||
// Clean-up Entry Group resource created in setUp() |
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.
delete this comment. the code is self-explanatory
// Clean-up code that will be executed after all tests | ||
public static void tearDown() throws Exception { | ||
// Clean-up Entry Group resource created in setUp() | ||
// Entry inside this Entry Group will be deleted automatically |
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.
this comment is required to clarify 1 "delete" vs. 2 "create"'s
Description
Fixes b/368505575
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
pom.xml
parent set to latestshared-configuration
mvn clean verify
requiredmvn -P lint checkstyle:check
requiredmvn -P lint clean compile pmd:cpd-check spotbugs:check
advisory only