Skip to content

Conversation

@muhsinking
Copy link

@muhsinking muhsinking requested a review from a team March 16, 2020 17:37
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 16, 2020
Copy link
Contributor

@lesv lesv left a comment

Choose a reason for hiding this comment

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

Tests are missing.

Copy link
Contributor

@lesv lesv left a comment

Choose a reason for hiding this comment

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

PTAL

Comment on lines +58 to +59
// PermissionDeniedException or NotFoundException are thrown if
// Entry does not exist.
Copy link
Contributor

Choose a reason for hiding this comment

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

Great that this is here -- Care to explain why one or the other?

Comment on lines +45 to +47
// Initialize client that will be used to send requests. This client only needs to be created
// once, and can be reused for multiple requests. After completing all of your requests, call
// the "close" method on the client to safely clean up any remaining background resources.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

Comment on lines +92 to +94
} catch (Exception e) {
System.out.printf("\nCannot delete template: %s", tagTemplateName);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to be explicit.

Comment on lines +121 to +125
.setUserSpecifiedSystem("onprem_data_system")
.setUserSpecifiedType("onprem_data_asset")
.setDisplayName("My awesome data asset")
.setDescription("This data asset is managed by an external system.")
.setLinkedResource("//my-onprem-server.com/dataAssets/my-awesome-data-asset")
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, we have a preference for samples to use UUID's, possibly in conjunction w/ text. We often have multiple tests running at the same time.

Comment on lines +204 to +207
// AlreadyExistsException's are thrown if EntryGroup or Entry already exists.
// IOException's are thrown when unable to create the DataCatalogClient,
// for example an invalid Service Account path.
System.out.println("Error creating entry:\n" + e.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this!

@muhsinking muhsinking closed this Mar 16, 2020
@muhsinking
Copy link
Author

Backing off on this change and waiting for the sample author to add tests. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants