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.

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

Update testing to new approach #18

Merged
merged 1 commit into from
Oct 11, 2019
Merged

Update testing to new approach #18

merged 1 commit into from
Oct 11, 2019

Conversation

ivankorn
Copy link
Contributor

@ivankorn ivankorn commented Oct 1, 2019 •
edited
Loading

Fixes #17

  • Migrated to Cloud Build as documented here.

Please note following deviations from migration document:

  • for_each instead of count.index is used to create a number of instances of resource.
  • pause of 30 sec (determined experimentally) is used to wait for APIs to get enabled as a workaround for the provider issue
  • pause of 30 sec (determined experimentally) is used to wait for permissions to get granted as a workaround for the provider issue

paulpalamarchuk
paulpalamarchuk suggested changes Oct 1, 2019
View reviewed changes
Copy link

@paulpalamarchuk paulpalamarchuk left a comment

Choose a reason for hiding this comment

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

I'm not sure if we need to keep old-style tests (bats)

build/int.cloudbuild.yaml Outdated Show resolved Hide resolved
kopachevsky
kopachevsky suggested changes Oct 1, 2019
View reviewed changes
test/setup/make_source.sh Outdated Show resolved Hide resolved
ingwarr
ingwarr previously approved these changes Oct 1, 2019
View reviewed changes
ivankorn
ivankorn commented Oct 1, 2019
View reviewed changes
Copy link
Contributor Author

@ivankorn ivankorn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if we need to keep old-style tests (bats)

@morgante requested keeping bats.

test/integration/boolean_constraints/launch.sh Show resolved Hide resolved
test/setup/make_source.sh Show resolved Hide resolved
test/setup/outputs.tf Show resolved Hide resolved
@ivankorn ivankorn marked this pull request as ready for review October 1, 2019 18:07
averbuks
averbuks reviewed Oct 3, 2019
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
morgante
morgante suggested changes Oct 3, 2019
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
Makefile Show resolved Hide resolved
test/setup/make_source.sh Outdated
echo "export FOLDER_2_ID='$folder_2_id'"; \
echo "export PROJECT_ID='$project_id'"; \
echo "export PROJECT_EXCLUDE='$project_exclude'"; \
echo "export PROJECT_CONSTRAINT_DENY_ALL='constraints/compute.trustedImageProjects'"; \
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't have any static values in this file. make_source is only necessary for generating values that change.

Please move all the static value back to the old location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@morgante. I agree, the constraint variables have been mistakenly moved to dynamic ones.

@kopachevsky @nick4fake,
Since current Cloud Build integration with GitHub doesn't support logs, further development of Cloud Build integration with this module would require approved access to the mentioned infrastructure component on the client side.

@kopachevsky, your Team take over of the task or GL guidance on further actions on it is required. As always, I'll be more than happy to help with any challenges GL may face ;)

test/integration/list_constraints/launch.sh

# Constraints

# Applied to: project
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't remove these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@morgante, absolutely! The constraints should be moved back.

@morgante morgante self-assigned this Oct 10, 2019
@morgante morgante merged commit cbd7a3b into terraform-google-modules:master Oct 11, 2019
@morgante morgante mentioned this pull request Oct 11, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@morgante morgante morgante requested changes

@averbuks averbuks averbuks left review comments

@ingwarr ingwarr ingwarr left review comments

@kopachevsky kopachevsky Awaiting requested review from kopachevsky

@paulpalamarchuk paulpalamarchuk Awaiting requested review from paulpalamarchuk

Assignees

@morgante morgante

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update testing to new approach
6 participants
@ivankorn @morgante @kopachevsky @ingwarr @averbuks @paulpalamarchuk