feat: add support for GitHub and gitlab in helper #1442
Conversation
daniel-cit
changed the title
daniel-cit
requested review from
a team,
eeaton,
rjerrems and
sleighton2022
as code owners
|
@apeabody Could you PTAL? |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for GitHub Actions and GitLab CI/CD as alternatives to Google Cloud Build for the deployment helper. The changes are extensive, involving significant refactoring to modularize CI/CD-specific configurations, the introduction of an Executor interface to abstract CI/CD system interactions, and updates to documentation and helper scripts. The overall design is solid. However, I've identified a few critical issues: a potential modification of the source code directory, a security risk of leaking secrets in logs, and a bug in the build-type selection script that could lead to an inconsistent state. Addressing these points will greatly improve the robustness and security of the new functionality.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for GitHub and GitLab CI/CD pipelines to the foundation deployer, which is a significant feature enhancement. The changes are well-structured, using an Executor interface to abstract the CI/CD provider logic and refactoring the Terraform files to be modular based on the chosen build type. This greatly improves the flexibility of the deployment tool. My review includes a few suggestions to enhance maintainability, such as addressing duplicated logic between a shell script and Go code, and refactoring a complex function. I also suggest adding a comment to the Makefile to clarify a change that disables parallel execution.
| # Deactivate all other build types to ensure a clean state | ||
| for type in "${build_types[@]}"; do | ||
| if [[ "$type" != "$TARGET_BUILD" ]]; then | ||
| find "$BASE_PATH" -name "*_$type.tf" -print0 | while IFS= read -r -d $'\0' file; do | ||
| if [ -f "$file" ]; then | ||
| new_name="${file}.example" | ||
| echo "Deactivating: renaming \"$file\" to \"$new_name\"" | ||
| mv "$file" "$new_name" | ||
| fi | ||
| done | ||
| fi | ||
| done | ||
|
|
||
| # Activate the target build type | ||
| find "$BASE_PATH" -name "*_$TARGET_BUILD.tf.example" -print0 | while IFS= read -r -d $'\0' file; do | ||
| base_name="${file%.tf.example}" | ||
| new_name="$base_name.tf" | ||
|
|
||
| echo "Activating: renaming \"$file\" to \"$new_name\"" | ||
| mv "$file" "$new_name" | ||
| done |
There was a problem hiding this comment.
This script's logic for activating/deactivating build types by renaming files is also implemented in Go in helpers/foundation-deployer/utils/files.go (RenameBuildFiles function). This duplication could be a maintenance risk, as changes to the file naming convention would need to be updated in both places. To improve maintainability, consider adding a comment here pointing to the Go implementation as the source of truth, or vice-versa.
| docker_test_lint: | ||
| docker run --rm -it \ | ||
| -e ENABLE_PARALLEL=1 \ | ||
| -e ENABLE_PARALLEL=0 \ |
There was a problem hiding this comment.
The change from ENABLE_PARALLEL=1 to 0 disables parallel execution for the linting tests. While this might be necessary to fix a flaky test or a tool limitation, it can slow down the CI pipeline. It would be helpful to add a comment on the line above explaining why parallel execution is disabled, for future maintainability.
There was a problem hiding this comment.
the make file target docker_test_lint is not used in the CI/CD pipeline
| "github.com/terraform-google-modules/terraform-example-foundation/test/integration/testutils" | ||
| ) | ||
|
|
||
| func buildGitLabCICDImage(t testing.TB, s steps.Steps, tfvars GlobalTFVars, c CommonConf) error { |
There was a problem hiding this comment.
The buildGitLabCICDImage function is called from within DeployBootstrapStage for GitLab setups. This seems to be a one-off setup step. Embedding it within the bootstrap deployment logic could be confusing for future maintainers. It might be clearer to either make this a more explicit, separate step in the main deployer logic (e.g., a pre-flight check or setup stage), or to add a comment explaining why this build is a necessary prerequisite for the GitLab bootstrap stage.
| } | ||
|
|
||
| func copyStepCode(t testing.TB, conf utils.GitRepo, foundationPath, checkoutPath, repo, step, customPath string) error { | ||
| func copyStepCode(t testing.TB, conf utils.GitRepo, foundationPath, checkoutPath, repo, step, customPath, buildType string) error { |
There was a problem hiding this comment.
The copyStepCode function has grown quite complex with the addition of logic for different build types. It now handles copying not just the stage code, but also CI/CD pipeline definitions and the policy library. This mixing of concerns could make the function harder to maintain. Consider refactoring this into smaller, more focused functions, for example by extracting the CI/CD file setup into a separate helper function.
2 participants
This PR adds support for GitHub Action and GitLab workflows deploy in deploy helper