Skip to content

fix: respect network replacement configured through manifest#3352

Merged
mergify[bot] merged 9 commits intoaws:mainlinefrom
Lou1415926:fix/manifest/vpc-placmenet-has-default
Mar 14, 2022
Merged

fix: respect network replacement configured through manifest#3352
mergify[bot] merged 9 commits intoaws:mainlinefrom
Lou1415926:fix/manifest/vpc-placmenet-has-default

Conversation

@Lou1415926
Copy link
Copy Markdown
Contributor

This PR fixes #3346 by:

  1. Remove the custom unmarshal logic of network field. The logic preset the network's placement value, which causes the bug that's seen in Environment overrides for network are overwriting base definitions. #3346.
  2. Use const instead of var for placement types.
    By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

@Lou1415926 Lou1415926 requested a review from a team as a code owner March 11, 2022 20:08
@Lou1415926 Lou1415926 requested review from huanjani and removed request for a team March 11, 2022 20:08
Comment thread internal/pkg/manifest/workload.go
Comment thread internal/pkg/manifest/workload.go Outdated
Comment thread internal/pkg/manifest/workload.go Outdated
Comment on lines +534 to +535
// PlacementP converts a string to a `Placement` type and returns its pointer.
func PlacementP(p string) *Placement {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of instead the following approach:

package manifest

// Placement represents VPC subnet placement.
type Placement string

const (
   PublicSubnetPlacement Placement    = "public"
   PrivateSubnetPlacement Placement = "private"
)

@Lou1415926 Lou1415926 added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Mar 14, 2022
@mergify mergify Bot merged commit 79126e4 into aws:mainline Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Environment overrides for network are overwriting base definitions.

4 participants