Modify mesh-task and controller #188
Conversation
Ganeshrockz
changed the base branch from
main
to
ganeshrockz/dataplane-arch-changes
Ganeshrockz
requested review from
a team,
sarahethompson,
alvin-huang,
kisunji,
cthain and
pglass
and removed request for
a team
| type = any | ||
| default = {} | ||
| description = <<-EOT | ||
| This accepts HTTP specific TLS configuration based on the `consulServers.http` schema present in https://github.com/hashicorp/consul-ecs/blob/main/config/schema.json. |
There was a problem hiding this comment.
Given that we release the projects separately as tagged versions, I don't think that we should be pointing to the ECS config schema on main. It may deviate from the versioned file supported by the module. I don't have a good alternative suggestion but I'll try to think through this.
There was a problem hiding this comment.
We can link them to the Consul docs: https://developer.hashicorp.com/consul/docs/ecs/configuration-reference, which are at least versioned by Consul versions
There was a problem hiding this comment.
Oh, we can use the Git tag (v0.6.0) in place of main. We can even link to the specific line of code containing the field, too. Example:
If we do this, we need to update the release process, and ensure we set this set correctly for each release branch. We'd need to set this to match the default consul_ecs_image version
There was a problem hiding this comment.
Sure. I'll keep a note of this
|
@pglass I've addressed almost all the comments except the one that talks about the version. Please re-review it whenever possible. Thanks |
There was a problem hiding this comment.
Awesome job on this @Ganeshrockz! I left a few minor comments/suggestions. Nothing blocking.
test/acceptance/tests/validation/terraform/envoy-readiness-port-validate/main.tf
Show resolved
Hide resolved
| t.Cleanup(func() { | ||
| _, _ = terraform.DestroyE(t, terraformOptions) | ||
| }) | ||
| terraform.InitAndPlan(t, terraformOptions) |
There was a problem hiding this comment.
Should we be validating the output of the plan? This test only checks that the plan works (which is a good start) but it doesn't verify that the plan matches our expectations.
This is already a huge PR so maybe this can be addressed in a follow up?
There was a problem hiding this comment.
Most of the validation tests here were just moved to this new location (which is great 👌). So agreed, but I feel like improvements to the existing validation tests can wait for a follow up.
There was a problem hiding this comment.
Will keep a note of this as well.
Changes proposed in this PR:
acl-controllermodule to justcontrollerHow I've tested this PR:
Unit and integration tests.
How I expect reviewers to test this PR:
Checklist: