Skip to content

Container Analysis samples #2232

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
Closed
wants to merge 28 commits into from
Closed

Container Analysis samples #2232

wants to merge 28 commits into from

Conversation

daniel-sanche
Copy link
Member

added Python samples for the Container Analysis GA

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 20, 2019
@daniel-sanche daniel-sanche force-pushed the container-analysis-ga branch from 017d257 to 95ba536 Compare June 20, 2019 20:37
@leahecole
Copy link
Collaborator

Disclaimer: I have NO container_analysis experience, but have used this repo before :) Here's how running this went:

-Pretty please add a README so users know how you want them to run your code (that they need to install the requirements, let them know what needs to be enabled in their GCP Project, etc., any environment vars). Given that I've never used container analysis, I failed a bunch before I was able to successfully run stuff

  • When running pip install -r requirements.txt in a virtual environment on your branch, I run into this dependency error (although when I pulled the upstream into my clone of your repo and rebased your work on top of it, it was okay)
ERROR: grafeas 0.1.0 has requirement google-api-core[grpc]<2.0.0dev,>=1.6.0, but you'll have google-api-core 0.1.4 which is incompatible.
ERROR: google-cloud-containeranalysis 0.1.0 has requirement google-api-core[grpc]<2.0.0dev,>=1.4.1, but you'll have google-api-core 0.1.4 which is incompatible.
  • the test_pubsub failed for me with a 404 resource not found resource=container-analysis-occurrences-v1beta1 - this is probably on me, and a README may have helped me not mess this up, but in case it's not me, double check that this test can run independently of the others

@daniel-sanche
Copy link
Member Author

@leahecole I added a README, let me know if that looks better. Your pubsub issues may have been because the team recently switched from the beta topic to the GA one, so I updated the sample to use the new topic. Let me know what you think

@leahecole leahecole self-requested a review July 3, 2019 23:00
leahecole
leahecole requested changes Jul 3, 2019
View reviewed changes
Copy link
Collaborator

@leahecole leahecole left a comment

Choose a reason for hiding this comment

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

pubsub test is also still giving me an error, and I wonder if it has to do with the incorrectly resolved dependency

container_registry/container_analysis/README.md Outdated
1. **Set Environment Variables**

```
$ export GOOGLE_CLOUD_PROJECT="YOUR_PROJECT_ID"
Copy link
Collaborator

Choose a reason for hiding this comment

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

GOOGLE_CLOUD_PROJECT -> GCLOUD_PROJECT

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should users also set their application credentials here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that required? I don't have it set in my terminal and it's working, I assume because I authenticated locally with "gcloud login"

I'm not really sure how much detail is required for set up here, so I copied this readme from a different project

@daniel-sanche daniel-sanche force-pushed the container-analysis-ga branch from ea1e1d9 to e8f79f8 Compare July 4, 2019 01:11
@daniel-sanche daniel-sanche added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 4, 2019
@daniel-sanche
Copy link
Member Author

Note that the tests aren't actually running, since this PR is based on a fork. I forgot that quirk of this repo

Maybe after we resolve these comments I can close this and open another one as an internal branch? Let me know what you think

@leahecole
Copy link
Collaborator

Ooo I forgot that's a quirk of this repo also. @dansanche I think that after resolution, opening another one and linking it here in the comments is a fabulous idea.

@daniel-sanche
Copy link
Member Author

@leahecole I believe I resolved the issues you mentioned, except setting application credentials. Let me know if you think that should be added

@leahecole
Copy link
Collaborator

Sorry, didn't process that properly. I think adding the application creds is a good idea because right now it throws a warning if you don't

@daniel-sanche
Copy link
Member Author

sounds good, I'll add it to the new PR

@daniel-sanche daniel-sanche mentioned this pull request Jul 8, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leahecole leahecole leahecole requested changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daniel-sanche @leahecole @googlebot