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

Monitoring: Fix environment variables for VPC tests. #8302

Merged
merged 6 commits into from
Aug 22, 2019

Conversation

busunkim96
Copy link
Contributor

Follow up to #8295.

CC @steinwaywhw

@busunkim96 busunkim96 added testing api: monitoring Issues related to the Cloud Monitoring API. labels Jun 13, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 13, 2019
@busunkim96 busunkim96 changed the title Fix environment variables for VPC tests. Monitoring: Fix environment variables for VPC tests. Jun 13, 2019
@steinwaywhw
Copy link
Contributor

Hi @busunkim96 sorry I miscommunicated in our last thread. Could you keep the environment variable names as is without changing them? I believe the VPC team sets "PROJECT_ID" when they run the tests.

@steinwaywhw
Copy link
Contributor

I think @bmccutchon knows better about what environment variables the VPCSC team needs.

@busunkim96
Copy link
Contributor Author

I changed it as we also use PROJECT_ID as our default system test project. I'll poke around a bit more to see what the best way to sort this out is. Thanks for the heads up!

@bmccutchon
Copy link
Contributor

Yes, we do set PROJECT_ID in VPCSC tests.

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jun 20, 2019
@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Jul 3, 2019
@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 30, 2019
@tseaver
Copy link
Contributor

tseaver commented Jul 30, 2019

Both Container Analysis and Grafeas jobs failed with "Test Fusion Url (This url will not work until build finishes)". I have tagged Kokoro to re-run the tests.

@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 30, 2019
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

ISTM that the whole TestVPCServiceControl testcase class is conditional on the environment variables being set. Would we be better off moving the pytest.mark.skipif stuff to decorate the class, rather than the individual methods?

@bmccutchon
Copy link
Contributor

We (VPCSC) would actually prefer it if as many of these tests as possible are not skipped on your CI so that we can more easily tell whether a failure is VPCSC-related. (If it's also failing on your CI, it's almost certainly not a VPCSC error.)

@busunkim96
Copy link
Contributor Author

@bmccutchon That sounds good. This also came up for the Asset VPC tests. I'll work on resolving the environment variable/test runs today.

@busunkim96 busunkim96 self-assigned this Aug 7, 2019
@busunkim96 busunkim96 closed this Aug 7, 2019
@busunkim96 busunkim96 reopened this Aug 7, 2019
Copy link
Contributor Author

@busunkim96 busunkim96 left a comment

Choose a reason for hiding this comment

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

@bmccutchon I'm still having a bit of trouble rationalizing the CI setup. Could you look over this and make sure I have the correct understanding?

PROJECT_ID - a project inside the VPCSC perimeter
GOOGLE_CLOUD_TESTS_VPCSC_OUTSIDE_PERIMETER_PROJECT - a project outside the VPCSC perimeter
GOOGLE_CLOUD_TESTS_IN_VPCSC - "true" in the VPCSC testing environment

Once this is correct I'll make another PR to fix the other VPCSC tests.

Thanks!

monitoring/noxfile.py Outdated Show resolved Hide resolved
monitoring/tests/system/test_vpcsc.py Show resolved Hide resolved
@bmccutchon
Copy link
Contributor

Your understanding is correct.

@busunkim96 busunkim96 requested a review from a team August 21, 2019 23:13
@busunkim96 busunkim96 requested a review from plamut August 22, 2019 17:06
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

The resolution of my comment is good, approving that. 👍

Cannot say much about the rest, not familiar enough with it, I'll leave that to others.

@busunkim96 busunkim96 merged commit c802ba3 into googleapis:master Aug 22, 2019
HemangChothani pushed a commit to HemangChothani/google-cloud-python that referenced this pull request Aug 29, 2019
emar-kar pushed a commit to MaxxleLLC/google-cloud-python that referenced this pull request Sep 18, 2019
@busunkim96 busunkim96 deleted the monitoring-update-environ branch January 30, 2020 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: monitoring Issues related to the Cloud Monitoring API. cla: yes This human has signed the Contributor License Agreement. testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants