-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Update integration tests to use new approach #262
Update integration tests to use new approach #262
Conversation
d3b9180
to
26db278
Compare
c13d1b6
to
80c5dee
Compare
80c5dee
to
5e604a0
Compare
test/setup/iam.tf
Outdated
|
||
locals { | ||
int_required_roles = [ | ||
"roles/owner", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to do not use wide rolesthat includes other permissions like comute.viewer etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it works, I think minimization of permissions is next low-priority task
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
role removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not removed. Please either fix it or say why you chose not to.
c3532a8
to
dd1fae1
Compare
dd1fae1
to
59f3d78
Compare
Makefile
Outdated
docker run --rm -it \ | ||
-v $(CURDIR):/workspace \ | ||
$(REGISTRY_URL)/${DOCKER_IMAGE_DEVELOPER_TOOLS}:${DOCKER_TAG_VERSION_DEVELOPER_TOOLS} \ | ||
/bin/bash -c 'source /usr/local/bin/task_helper_functions.sh && /workspace/helpers/generate.sh && generate' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not source an extra file. This is not necessary, as I explained in chat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
build/lint.cloudbuild.yaml
Outdated
# limitations under the License. | ||
|
||
steps: | ||
- id: 'lint-pull-image' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to do this. @aaron-lane I know you had suggested it but I think it's a bad idea. If we split them out like this, we'll have to copy every linting step into every repo and maintain it separately.
build/lint.cloudbuild.yaml
Outdated
waitFor: ['lint-pull-image'] | ||
- id: 'lint-terraform' | ||
name: 'gcr.io/cloud-foundation-cicd/$_DOCKER_IMAGE_DEVELOPER_TOOLS:$_DOCKER_TAG_VERSION_DEVELOPER_TOOLS' | ||
args: ['/bin/bash', '-c', 'source /usr/local/bin/task_helper_functions.sh && source /workspace/helpers/generate.sh && check_terraform'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't source custom file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
helpers/generate.sh
Outdated
./helpers/generate_modules/generate_modules.py | ||
} | ||
|
||
function check_generate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this function rewritten to be different from master? I'd like an explanation of why these changes were done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- At the moment we don't have function to generate submodules from
autorgen
directory in docker-image. - I've changed function
find_files
(added path./autogen
) to exclude that dir from terraform format checks
test/setup/iam.tf
Outdated
|
||
locals { | ||
int_required_roles = [ | ||
"roles/owner", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not removed. Please either fix it or say why you chose not to.
a571209
1556a25
to
40a918e
Compare
/workspace/helpers/generate_modules/generate_modules.py | ||
} | ||
|
||
function check_generate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulpalamarchuk I still don't understand why this differs so much from master. It's not clear to me why this would need to be different from the logic we already have [written](Right, what I don't understand is why the logic for this function is so different from what's currently implemented on master. Instead of needing to ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@morgante I changed this function to avoid errors on CI, in case of using git-diff
to compare files. Error: fatal: not a git repository (or any parent up to mount point /)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok it's workable then.
40a918e
to
d966358
Compare
* Fix terraform-google-modules#245 * Fixed lint-tests * Backwards compatibility of the `generate` function * Disabled kitchen test `stub-domains-private-local` (issue terraform-google-modules#264)
d966358
to
f7d2a6c
Compare
…move_tests_to_cloudbuild Update integration tests to use new approach
generate
functionstub-domains-private-local
(issue Integration test "stub-domains-private-local" fails, trying to reach internal network #264)deploy_service
andnode_pool
(issue Testdeploy_service
andnode_pool
fail verification on CI #274)