Skip to content

Container Analysis Beta #1194

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Aug 31, 2018
Merged

Container Analysis Beta #1194

merged 7 commits into from
Aug 31, 2018

Conversation

daniel-sanche
Copy link
Member

Updated the Container Analysis samples for the upcoming beta release.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 24, 2018
@daniel-sanche daniel-sanche force-pushed the drydock-beta branch 3 times, most recently from 4168a2f to 11f11c1 Compare August 24, 2018 01:37
daniel-sanche
daniel-sanche commented Aug 24, 2018
View reviewed changes
container-registry/container-analysis/src/main/java/com/example/containeranalysis/Samples.java Outdated
throws Exception {
public static Subscription createOccurrenceSubscription(String subId, String projectId)
throws IOException, StatusRuntimeException {
// This topic id will automatically receive messages when Occurrences are added or modified
String topicId = "resource-notes-occurrences-v1alpha1";
Copy link
Member Author

Choose a reason for hiding this comment

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

@vtsao Is there a new topic Id for beta?

Copy link

Choose a reason for hiding this comment

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

Yup:
container-analysis-occurrences-v1beta1

vtsao
vtsao previously approved these changes Aug 24, 2018
View reviewed changes
container-registry/container-analysis/src/main/java/com/example/containeranalysis/Samples.java Outdated
}
public static Occurrence createOccurrence(GrafeasV1Beta1Client client, String imageUri,
String parentNoteId,String projectId) {
final String parentNoteName = client.formatNoteName(projectId, parentNoteId);
Copy link

Choose a reason for hiding this comment

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

Variable can just be called noteName.

Copy link

Choose a reason for hiding this comment

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

And also just "noteId".

container-registry/container-analysis/src/main/java/com/example/containeranalysis/Samples.java Outdated
* Makes an update to an existing occurrence
*
* Pushes an update to an Occurrence that already exists on the server
* @param client The Grafeas client used to perform the API requests.
* @param occurrenceName the name of the occurrence to delete.
* format: "projects/{projectId}/occurrences/{occurrence_id}"
Copy link

Choose a reason for hiding this comment

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

All the forms should be stylized as:
"projects/[PROJECT_ID]/occurrences/[OCCURRENCE_ID]"

container-registry/container-analysis/src/main/java/com/example/containeranalysis/Samples.java Outdated
throws Exception {
public static Subscription createOccurrenceSubscription(String subId, String projectId)
throws IOException, StatusRuntimeException {
// This topic id will automatically receive messages when Occurrences are added or modified
String topicId = "resource-notes-occurrences-v1alpha1";
Copy link

Choose a reason for hiding this comment

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

Yup:
container-analysis-occurrences-v1beta1

vtsao
vtsao reviewed Aug 27, 2018
View reviewed changes
container-registry/container-analysis/src/main/java/com/example/containeranalysis/Samples.java Outdated
try (ContainerAnalysisClient client = ContainerAnalysisClient.create()) {
final String noteName = client.formatNoteName(projectId, noteId);
public static Note getNote(GrafeasV1Beta1Client client, String noteId, String projectId) {
final String noteName = client.formatNoteName(projectId, noteId);
Copy link

Choose a reason for hiding this comment

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

FYI the Java lib has been updated to now have the methods with the resource name types:
https://github.com/GoogleCloudPlatform/google-cloud-java/tree/master/google-cloud-clients/google-cloud-containeranalysis/src/main/java/com/google/cloud/devtools/containeranalysis/v1beta1

E.g., instead of using the formatter to create a string, you can do:
NoteName name = NoteName.of("[PROJECT]", "[NOTE]");

to use the NoteName type.

Copy link

Choose a reason for hiding this comment

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

Same for all other resource types.

@daniel-sanche daniel-sanche force-pushed the drydock-beta branch 2 times, most recently from b9b441f to 317b7e3 Compare August 27, 2018 19:32
@vtsao
Copy link

vtsao commented Aug 27, 2018

LGTM

kurtisvg
kurtisvg suggested changes Aug 29, 2018
View reviewed changes
...ner-registry/container-analysis/src/test/java/com/example/containeranalysis/SamplesTest.java Outdated

@Test
public void testCreateNote() throws Exception {

Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty line

...ner-registry/container-analysis/src/test/java/com/example/containeranalysis/SamplesTest.java Outdated
Note n = Samples.getNote(client, noteId, PROJECT_ID);

assertEquals(n.getName(), noteObj.getName());

Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty line

...ner-registry/container-analysis/src/test/java/com/example/containeranalysis/SamplesTest.java Outdated
Samples.deleteNote(client, noteId, PROJECT_ID);
try {
Samples.getNote(client, noteId, PROJECT_ID);
//above should throw, because note was deleted
Copy link
Contributor

Choose a reason for hiding this comment

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

// Above

Copy link
Contributor

Choose a reason for hiding this comment

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

This file seems to have a bunch of check-style violations on it for some reason - can you try running checkstyle on it manually and addressing the issues? mvn checkstyle:check

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm that's strange, I'm not seeing any on my end, and Kokoro isn't blocking anything either. Any ideas what might be going on? I'm not too familiar with maven

...ner-registry/container-analysis/src/test/java/com/example/containeranalysis/SamplesTest.java Outdated
Samples.getNote(client, noteId, PROJECT_ID);
//above should throw, because note was deleted
fail("note not deleted");
} catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to catch every Exception - can we only catch the correct error instead?

...ner-registry/container-analysis/src/test/java/com/example/containeranalysis/SamplesTest.java Outdated
@@ -0,0 +1,256 @@
/*
* Copyright 2018 Google Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be Google LLC for all new headers

Copy link
Member Author

Choose a reason for hiding this comment

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

The checkstyle plugin seems to enforce using Inc for me. Am I using an outdated version or something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure the following parent is in your pom:

    <!--
      The parent pom defines common style checks and testing strategies for our samples.
      Removing or replacing it should not affect the execution of the samples in anyway.
    -->
    <parent>
        <groupId>com.google.cloud.samples</groupId>
        <artifactId>shared-configuration</artifactId>
        <version>1.0.9</version>
    </parent>

Its should accept Inc. or LLC - no punctuation on the second.

Copy link
Member Author

@daniel-sanche daniel-sanche Aug 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I did have that in my pom, but it looks like LLC works if I update to 1.0.10, so I switched to that instead

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah that's weird. We must have some other out of dates in this repo.

...ner-registry/container-analysis/src/test/java/com/example/containeranalysis/SamplesTest.java Outdated

@Test
public void testCreateOccurrence() throws Exception {

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove emtpy line

@kurtisvg kurtisvg added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 31, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 31, 2018
@daniel-sanche daniel-sanche merged commit 4b4328e into GoogleCloudPlatform:master Aug 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtsao vtsao vtsao left review comments

@kurtisvg kurtisvg kurtisvg approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@daniel-sanche @vtsao @kurtisvg @googlebot @kokoro-team