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

Fixes #180: Add tests for beta submodules/examples #219

Merged
merged 4 commits into from
Nov 29, 2019
Merged

Fixes #180: Add tests for beta submodules/examples #219

merged 4 commits into from
Nov 29, 2019

Conversation

bohdanyurov-gl
Copy link
Contributor

@bohdanyurov-gl bohdanyurov-gl commented Jul 26, 2019 •
edited by aaron-lane

Fixes #180
Fixes #185

Tests for beta GKE features.

@morgante morgante mentioned this pull request Jul 26, 2019
Closed
@bohdanyurov-gl bohdanyurov-gl marked this pull request as ready for review July 26, 2019 19:23
@morgante
Copy link
Contributor

morgante commented Aug 7, 2019

@bohdanyurov-gl Looks like tests are failing. Can you check locally?

@nick4fake
Copy link
Contributor

@morgante we've discussed this with @aaron-lane today. Looks like there is another permission missing from ci fixtures

@morgante
Copy link
Contributor

Looks like there are still issues unrelated to permissions: https://concourse.infra.cft.tips/builds/6158

       Error: Invalid index
       
         on ../../../modules/beta-public-cluster/main.tf line 262, in locals:
        262:   cluster_master_auth_list_layer2 = local.cluster_master_auth_list_layer1[0]

@nick4fake
Copy link
Contributor

@morgante @aaron-lane has told me there was an issue with permission, here is pr: GoogleCloudPlatform/cloud-foundation-toolkit#280

Regarding the failure you've noted - it should not be related to my work. Tests pass smoothly locally

@nick4fake
Copy link
Contributor

image

@aaron-lane aaron-lane added the enhancement New feature or request label Aug 16, 2019
ivankorn
ivankorn previously requested changes Aug 20, 2019
View reviewed changes
Copy link
Contributor

@ivankorn ivankorn left a comment

Choose a reason for hiding this comment

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

@bohdanyurov-gl
How do you feel about fixing linters and integration tests
So it could pass the checks and be merged?
This PR blocks integration tests for #228

@kopachevsky
Copy link
Contributor

@bohdanyurov-gl some strange commit in PR, probably result of incorrect rebase/merge

kopachevsky
kopachevsky reviewed Aug 21, 2019
View reviewed changes
Copy link
Contributor

@kopachevsky kopachevsky left a comment

Choose a reason for hiding this comment

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

remove needless merge commit

@nick4fake
Copy link
Contributor

@kopachevsky I haven't added it. Waiting for @morgante's response

@nick4fake
Copy link
Contributor

@bohdanyurov-gl
How do you feel about fixing linters and integration tests
So it could pass the checks and be merged?
This PR blocks integration tests for #228

Linters and integration tests are not related to my work. Please wait for other PRs to be merged:
#238

kopachevsky
kopachevsky previously approved these changes Sep 11, 2019
View reviewed changes
morgante
morgante suggested changes Oct 30, 2019
View reviewed changes
Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

Please rebase on master and add the new tests in the Cloud Build yaml.

@nick4fake
Copy link
Contributor

@morgante Integration tests are failing due to outdated google-beta provider. It is going to be fixed in this PR:
#295

morgante
morgante previously requested changes Nov 20, 2019
View reviewed changes
Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

Test fixtures should not directly invoke or create resources (as the other fixtures show).

Instead, your new fixture should merely wrap the existing beta cluster example: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/tree/master/examples/simple_regional_beta

@aaron-lane aaron-lane assigned aaron-lane and unassigned morgante Nov 29, 2019
@aaron-lane aaron-lane dismissed stale reviews from morgante and ivankorn November 29, 2019 20:32

The example has been wrapped. Additional fixture resources can be moved to the example subsequently.

@aaron-lane aaron-lane merged commit 27786a6 into terraform-google-modules:master Nov 29, 2019
CPL-markus pushed a commit to WALTER-GROUP/terraform-google-kubernetes-engine that referenced this pull request Jul 15, 2024
Merge pull request terraform-google-modules#219 from bohdanyurov-gl/f…
926fcb4 
…ix/180

Fixes terraform-google-modules#180: Add tests for beta submodules/examples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaron-lane aaron-lane aaron-lane approved these changes

@morgante morgante morgante left review comments

@kopachevsky kopachevsky kopachevsky left review comments

@adrienthebo adrienthebo Awaiting requested review from adrienthebo

@Jberlinsky Jberlinsky Awaiting requested review from Jberlinsky

@paulpalamarchuk paulpalamarchuk Awaiting requested review from paulpalamarchuk

@ivankorn ivankorn Awaiting requested review from ivankorn

Assignees

@nick4fake nick4fake

@aaron-lane aaron-lane

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add database_encryption tests for terraform 0.12 Add tests for beta submodules/examples
6 participants
@bohdanyurov-gl @morgante @nick4fake @kopachevsky @aaron-lane @ivankorn