-
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
Add Data Catalog createEntry samples and tests. #1638
Add Data Catalog createEntry samples and tests. #1638
Conversation
Are these samples being authored with the intent to include them as part of our docs somewhere? or are you just writing them because you thought the current set of samples is missing this functionality? |
Hi @kurtisvg, yes, they are being authored with the intent to include them as part of google docs. And also the current set of samples is missing this functionality. |
datacatalog/cloud-client/src/main/java/com/example/datacatalog/CreateFilesetEntry.java
Outdated
Show resolved
Hide resolved
datacatalog/cloud-client/src/main/java/com/example/datacatalog/CreateFilesetEntry.java
Outdated
Show resolved
Hide resolved
datacatalog/cloud-client/src/main/java/com/example/datacatalog/CreateFilesetEntry.java
Outdated
Show resolved
Hide resolved
try { | ||
dataCatalogClient.deleteEntry( | ||
EntryName.of(projectId, location, entryGroupId, entryId).toString()); | ||
} catch (Exception e) { |
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.
You shouldn't catch Exception - you should catch the specific error being thrown and show the user how to handle it correctly.
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.
Removed, will add new PRs for delete methods.
datacatalog/cloud-client/src/main/java/com/example/datacatalog/CreateFilesetEntry.java
Outdated
Show resolved
Hide resolved
// Delete any pre-existing Entry Group with the same name | ||
// that will be used in step 2. | ||
try { | ||
dataCatalogClient.deleteEntryGroup( |
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.
Same as comments above.
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.
Removed, will add new PRs for delete methods.
System.out.printf("\nEntry created with name: %s\n", entryResponse.getName()); | ||
|
||
|
||
} catch (Exception e) { |
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 Exception is too broad as well.
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.
Changed, also now explaining what could cause the exceptions.
|
||
private static String PROJECT_ID = System.getenv().get("GOOGLE_CLOUD_PROJECT"); | ||
private static String LOCATION = "us-central1"; | ||
private static String ENTRY_GROUP_ID = "fileset_entry_group"; |
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.
these should probably use a randomly generated UUID.
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.
Changed, using only the first 8 chars from UUID, since hyphens are not supported on those IDs.
Entry entry = Entry.newBuilder() | ||
.setDisplayName("My Fileset") | ||
.setDescription("This fileset consists of ....") | ||
.setGcsFilesetSpec(GcsFilesetSpec.newBuilder().addFilePatterns("gs://my_bucket/*") |
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.
Generally we try to prioritize splitting between different syntactic levels, and remain consistent:
.setGcsFilesetSpec(
GcsFilesetSpec.newBuilder().addFilePatterns("gs://my_bucket/*").build())
Or, if the line is too long:
.setGcsFilesetSpec(
GcsFilesetSpec.newBuilder()
.addFilePatterns("gs://my_bucket/*")
.build())
You can use google-java-format to format the entire file at once.
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.
Ran google-java-format, could you check?
datacatalog/cloud-client/src/main/java/com/example/datacatalog/CreateFilesetEntry.java
Outdated
Show resolved
Hide resolved
datacatalog/cloud-client/src/main/java/com/example/datacatalog/CreateFilesetEntry.java
Outdated
Show resolved
Hide resolved
datacatalog/cloud-client/src/main/java/com/example/datacatalog/CreateFilesetEntry.java
Show resolved
Hide resolved
datacatalog/cloud-client/src/main/java/com/example/datacatalog/CreateFilesetEntry.java
Show resolved
Hide resolved
datacatalog/cloud-client/src/main/java/com/example/datacatalog/CreateFilesetEntry.java
Show resolved
Hide resolved
datacatalog/cloud-client/src/main/java/com/example/datacatalog/CreateFilesetEntry.java
Outdated
Show resolved
Hide resolved
datacatalog/cloud-client/src/main/java/com/example/datacatalog/CreateFilesetEntry.java
Show resolved
Hide resolved
datacatalog/cloud-client/src/main/java/com/example/datacatalog/CreateFilesetEntry.java
Show resolved
Hide resolved
|
||
/** Integration (system) tests for {@link CreateEntryGroup}. */ | ||
@RunWith(JUnit4.class) | ||
@SuppressWarnings("checkstyle:abbreviationaswordinname") |
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.
Can you remove this line? We should try to avoid suppression style checks.
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.
removed
datacatalog/cloud-client/src/test/java/com/example/datacatalog/CreateEntryGroupTests.java
Outdated
Show resolved
Hide resolved
datacatalog/cloud-client/src/test/java/com/example/datacatalog/CreateFilesetEntryTests.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.
LGTM once tests pass
Hi @mesmacosta, This already has my approval. Do you need me to merge it for you? |
Yes @kurtisvg, can you do that? |
* Add Data Catalog createEntry samples and tests * Change second column name * Code review changes * Change region tag for the correct one * Removed tag from region tag since this sample does not tag the entry * ADD method overload to show parameter values examples * Code review changes * Code review changes * Code review and checkstyle changes * Clean up in a safer order
* Add Data Catalog createEntry samples and tests * Change second column name * Code review changes * Change region tag for the correct one * Removed tag from region tag since this sample does not tag the entry * ADD method overload to show parameter values examples * Code review changes * Code review changes * Code review and checkstyle changes * Clean up in a safer order
I would like to add samples for Data Catalog create_entry method, released on 0.28.0-alpha google-cloud-datacatalog version.