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.

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 list recommendations code sample #1789

Merged
merged 9 commits into from
Dec 7, 2019

Conversation

ouitavon
Copy link
Contributor

@ouitavon ouitavon commented Dec 6, 2019

No description provided.

@ouitavon ouitavon requested a review from a team December 6, 2019 01:27
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 6, 2019
@kurtisvg kurtisvg added the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 6, 2019
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 6, 2019
recommender/beta/cloud-client/pom.xml Outdated Show resolved Hide resolved
recommender/beta/cloud-client/pom.xml Outdated Show resolved Hide resolved
## Quickstart

### Setup
- Install [Maven](http://maven.apache.org/).
Copy link
Contributor

Choose a reason for hiding this comment

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

Preferable we can point to the "Setting up a Java environment" page on GCP which is a little more detailed on what the need for their environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, linked to Setting up a Java environment page for the first step.

public class ListRecommendations {

// List recommendations for a specified project, location, and recommender
public static void listRecommendations(String projectId, String location, String recommender) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a no-arg, overloaded function for this - see Function Structure

Copy link
Contributor

Choose a reason for hiding this comment

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

If location isn't required for testing (looks like it's set to global atm), please just inline in the sample (and add a comment with a link to where the rules around what region can be specified for recommender's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Went ahead and just use global and google.iam.policy.Recommender as default location and recommender.

}
}

public static void main(String... args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For snippets, we strongly prefer not to include CLI parsing as part of the snippet for maintenance reasons. Most users will find the sample from the docs for the snippet, and most often copy/paste the sample and use their IDE to execute - See Exception Handling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

System.out.println();
}
System.out.println("List recommendations successful");
} catch (IOException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IOException is usually a low level file/networking error and should be allowed to bubble up - we only want to catch errors if it's a response from the service or if it's something we are going to show the user how to recover from - see Exception Handling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, letting error bubble up.

ListRecommendationsRequest.newBuilder().setParent(parent).build();

// Send the request and print out each recommendation
for (Recommendation responseItem :
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks better, but please separate out the call from the iteration to make more clear. Often users are unsure when the API call actually happens with built requests, so it's helpful to be specific:

// Send the request to the service and receive the response
List<Recommendation> response = recommenderClient.listRecommendations(request);

// Print each recommendation
for (Recommendation recommendation : response.iterateAll()) {
  // ...
}
System.out.println("List recommendations successful");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separated out the request from the print loop.

// List recommendations for a specified project, location, and recommender
public static void listRecommendations(String projectId, String location, String recommender)
throws IOException {
RecommenderClient recommenderClient = RecommenderClient.create();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please enclose with a try-with-resources and add the comment from the Sample Format guide regarding using .close():

// 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.
try (DlpServiceClient dlp = DlpServiceClient.create()) {
  // Do something
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


// List IAM recommendations for GOOGLE_CLOUD_PROJECT environment variable
public static void listRecommendations() throws IOException {
String projectId = System.getenv("GOOGLE_CLOUD_PROJECT");
Copy link
Contributor

Choose a reason for hiding this comment

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

Assume the user wants to edit inline and run from IDE:

Suggested change
String projectId = System.getenv("GOOGLE_CLOUD_PROJECT");
// TODO(developer): Replace these variables before running the sample.
String projectId = "my-project-id";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

System.out.println("List recommendations successful");
}

public static void main(String... args) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should exclude main altogether - users can run the snippet by replacing the variables in the no-arg function call and triggering themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, also removed option to run from main in the readme.


@Test
public void listRecommendations() throws IOException {
ListRecommendations.listRecommendations();
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to update to listRecommendations(String projectId, String location, String recommender) - When move the env vars to this side, make sure to throw an error if they aren't there. See this example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated and added env var checks.

@kurtisvg kurtisvg merged commit 2e24257 into GoogleCloudPlatform:master Dec 7, 2019
Shabirmean pushed a commit that referenced this pull request Nov 11, 2022
* Add list recommendations code sample

* Update recommender/beta/cloud-client/pom.xml

Co-Authored-By: Kurtis Van Gent <31518063+kurtisvg@users.noreply.github.com>

* Update recommender/beta/cloud-client/pom.xml

Co-Authored-By: Kurtis Van Gent <31518063+kurtisvg@users.noreply.github.com>

* Address review comments

* Add additional comments for location and recommender

* Address CL comments
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.

4 participants