fix(perforce): restructure tests and fix pre-existing module bugs#870
Open
fix(perforce): restructure tests and fix pre-existing module bugs#870
Conversation
Contributor
📚 Documentation Preview✅ Preview deployed successfully! 🔗 Preview URL: https://aws-games.github.io/cloud-game-development-toolkit/preview-pr-870/ 🔒 Maintainer Action RequiredThe preview requires approval before it's accessible. A maintainer must approve the GitHub Pages deployment in the Environments section. Once approved, the preview will be accessible within 1-2 minutes. Build Information
This preview will be automatically deleted when the PR is merged or closed. |
This was referenced Feb 14, 2026
gabebatista
approved these changes
Feb 17, 2026
Contributor
gabebatista
left a comment
gabebatista
left a comment
There was a problem hiding this comment.
Looks good to me. Left some minor comments regarding readability but nothing that should block the merge.
| Resource = "*" | ||
| }] | ||
| }) | ||
| json = "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Action\":\"*\",\"Resource\":\"*\"}]}" |
Contributor
There was a problem hiding this comment.
I find the old version easier to read. Any reason this approach is better?
| Version = "2012-10-17" | ||
| Statement = [{ Effect = "Allow", Action = "*", Resource = "*" }] | ||
| }) | ||
| json = "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Action\":\"*\",\"Resource\":\"*\"}]}" |
Contributor
There was a problem hiding this comment.
same as before, I find the old one easier to read.
Test restructuring: - Flatten test directory (remove unit/ and integration/ subdirectories) - Delete integration tests (require AWS credentials and SSM parameters) - Replace jsonencode() in mock_data with raw JSON strings - Add explicit disabled flags for NLB/ALB/R53 in test variables - Fix FQDN/zone name mismatch in full_stack test data - Fix assertion resource names (lb_access_logs → shared_lb_access_logs_bucket) - Simplify tests/README.md Module bug fixes: - versions.tf: add netapp-ontap provider (required by p4-server submodule) - variables.tf: add null guards to all p4_server_config validation rules - variables.tf: fix inverted shared_lb_access_logs_bucket validation - variables.tf: fix route53_private_hosted_zone_name error message - lb.tf: require both NLB and ALB for NLB→ALB resources - sg.tf: require ALB for NLB→ALB egress rule - main.tf: add try() fallback for p4_code_review secret ARN references - outputs.tf: add null guard for p4_server_lambda_link_name
hwkiem
force-pushed
the
fix/perforce-test-infrastructure-and-bugs
branch
from
b75d680 to
af771a4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tests/unit/totests/Bug Fixes
Provider resolution: Add
netapp-ontapprovider toversions.tf(required by the p4-server FSxN submodule, prevented all tests from running).Null safety (
variables.tf):var.p4_server_config == null ||guards to all 5 validation rules so the module works when P4 Server is not deployedshared_lb_access_logs_bucketvalidation conditionroute53_private_hosted_zone_nameResource count conditions:
lb.tf: NLB→ALB target group, attachment, and HTTPS listener now require bothcreate_shared_network_load_balancer && create_shared_application_load_balancersg.tf: NLB→ALB egress rule now also requires ALBReference safety:
main.tf: Addtry()fallback forp4_code_review→module.p4_server[0]secret ARN references (allows Code Review without P4 Server)outputs.tf: Addvar.p4_server_config != nullguard forp4_server_lambda_link_nameTest Changes
jsonencode()calls in mock_data blocks with raw JSON stringscreate_shared_network_load_balancer = false, etc.) to test variableslb_access_logs→shared_lb_access_logs_bucket)tests/README.mdTest plan
terraform testinmodules/perforce/— all 14 tests pass (8 conditional + 6 shared)terraform fmt -check -recursive modules/perforce/— formatting OKterraform validateinmodules/perforce/— configuration valid