chore: retrofit --allow-vpc-ingress flag to manifest#3763
chore: retrofit --allow-vpc-ingress flag to manifest#3763mergify[bot] merged 11 commits intoaws:mainlinefrom
--allow-vpc-ingress flag to manifest#3763Conversation
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestCloudFormation_UpgradeEnvironment(t *testing.T) { |
There was a problem hiding this comment.
These shouldn't be part of the change after I'm able to rebase on #3758.
paragbhingre
left a comment
paragbhingre
left a comment
There was a problem hiding this comment.
I just have one concern otherwise looks good to me.
| Certificates []string `yaml:"certificates,omitempty"` | ||
| InternalALBSubnets []string `yaml:"subnets,omitempty"` | ||
| Certificates []string `yaml:"certificates,omitempty"` | ||
| SecurityGroupsConfig securityGroupsConfig `yaml:"security_groups,omitempty"` |
There was a problem hiding this comment.
Just wanted to bring to your notice that my PR to add security_group to env manifest also have similar secuirty_group field in the vpc.
I was just concerned about having the same naming with just extra s at the end of security_groups will be little confusing for the customers. What do you think about this?
There was a problem hiding this comment.
This is going to read
http:
private:
security_groups: # configs for the sg used on internal ALB
Whereas yours iirc would be
security_group:
# environmentsg configs
so I guess we're probably fine!!
There was a problem hiding this comment.
Yes you are correct. I was just making sure that 2 almost similar parameters shouldn't be confusing to the customers. But as you said they read differently in different sections should be okay.
There was a problem hiding this comment.
This is related to https://github.com/aws/copilot-cli/pull/3737/files#diff-4c63a9ce04347fa71ffd19988ac84f3eca7aacf36c414bb9e8f95c011b0f622dR327 . it is just from_cdn won't be valid for private.
http:
public:
security_groups:
ingress:
from_cdn: true| PrivateImportedCertARNs: e.importPrivateCertARNs(), | ||
| VPCConfig: e.vpcConfig(), | ||
| CustomInternalALBSubnets: e.internalALBSubnets(), | ||
| AllowVPCIngress: e.in.AllowVPCIngress, // TODO(jwh): fetch AllowVPCIngress from Manifest or SSM. |
There was a problem hiding this comment.
Do we need to remove AllowVPCIngress from e.in?
There was a problem hiding this comment.
we can't because we need to feed from e.in to manifest for env show --manifest
| type: Environment | ||
| observability: {container_insights: true} | ||
| http: {public: {certificates: [cert-1, cert-2]}} | ||
| http: {public: {certificates: [cert-1, cert-2]}, private: {security_groups: {allow_vpc_ingress: true}}} |
There was a problem hiding this comment.
should we do
http:
private:
security_groups:
ingress:
allow_vpc: true| Certificates []string `yaml:"certificates,omitempty"` | ||
| InternalALBSubnets []string `yaml:"subnets,omitempty"` | ||
| Certificates []string `yaml:"certificates,omitempty"` | ||
| SecurityGroupsConfig securityGroupsConfig `yaml:"security_groups,omitempty"` |
There was a problem hiding this comment.
This is related to https://github.com/aws/copilot-cli/pull/3737/files#diff-4c63a9ce04347fa71ffd19988ac84f3eca7aacf36c414bb9e8f95c011b0f622dR327 . it is just from_cdn won't be valid for private.
http:
public:
security_groups:
ingress:
from_cdn: true
4 participants
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.