Skip to content

fix: sc shouldn't affect gov cloud deployment#4390

Merged
mergify[bot] merged 2 commits intoaws:mainlinefrom
iamhopaul123:sc-gov-cloud-fix
Jan 20, 2023
Merged

fix: sc shouldn't affect gov cloud deployment#4390
mergify[bot] merged 2 commits intoaws:mainlinefrom
iamhopaul123:sc-gov-cloud-fix

Conversation

@iamhopaul123
Copy link
Copy Markdown
Contributor

@iamhopaul123 iamhopaul123 commented Jan 20, 2023

fixes #4353

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

@iamhopaul123 iamhopaul123 requested a review from a team as a code owner January 20, 2023 00:52
@iamhopaul123 iamhopaul123 requested review from dannyrandall and removed request for a team January 20, 2023 00:52
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 20, 2023 •
edited
Loading

🍕 Here are the new binary sizes!

Name New size (kiB) size (kiB) Delta (%)
macOS (amd) 48548 48440 +0.22
macOS (arm) 49200 49108 +0.19
linux (amd) 42704 42608 +0.23
linux (arm) 42052 41924 +0.31
windows (amd) 39460 39368 +0.23

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 20, 2023

Codecov Report

Merging #4390 (94fb239) into mainline (dffeb8d) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           mainline    #4390      +/-   ##
============================================
- Coverage     70.06%   70.03%   -0.03%     
============================================
  Files           261      262       +1     
  Lines         37240    37288      +48     
  Branches        266      266              
============================================
+ Hits          26092    26115      +23     
- Misses         9892     9917      +25     
  Partials       1256     1256
Impacted Files Coverage Δ
internal/pkg/cli/deploy/workload.go 55.36% <0.00%> (-0.98%) ⬇️
internal/pkg/cli/job_deploy.go 34.69% <0.00%> (-0.73%) ⬇️
internal/pkg/cli/svc_deploy.go 31.47% <0.00%> (-0.39%) ⬇️
internal/pkg/cli/svc_package.go 25.07% <0.00%> (-0.39%) ⬇️
internal/pkg/cli/deploy/override.go 79.16% <0.00%> (ø)
internal/pkg/workspace/workspace.go 74.40% <0.00%> (+0.24%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Lou1415926
Lou1415926 approved these changes Jan 20, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@Lou1415926 Lou1415926 left a comment

Choose a reason for hiding this comment

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

Thanks boss for the quick turnaround!

Lou1415926
Lou1415926 reviewed Jan 20, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@Lou1415926 Lou1415926 left a comment

Choose a reason for hiding this comment

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

Oh anything we need to change in those slow integ tests tagged with //go:build integration?

paragbhingre
paragbhingre approved these changes Jan 20, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@paragbhingre paragbhingre left a comment •
edited
Loading

Choose a reason for hiding this comment

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

🚢
Feel free to remove label after addressing Wanxian's comment.

@paragbhingre paragbhingre added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jan 20, 2023
efekarakus
efekarakus approved these changes Jan 20, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

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

Thank you boss!

efekarakus
efekarakus reviewed Jan 20, 2023
View reviewed changes
Comment thread internal/pkg/template/templates/workloads/partials/cf/service-base-properties.yml
Comment on lines +79 to +82
!If
- IsGovCloud
- !Ref AWS::NoValue
- Enabled: False
Copy link
Copy Markdown
Contributor

@efekarakus efekarakus Jan 20, 2023

Choose a reason for hiding this comment

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

dumb question but instead of a condition with !Ref AWS::NoValue why not do:

{{if .ServiceConnect}}
ServiceConnectConfiguration:

{{- end}}

This way we would get rid of an extra CFN condition?

Copy link
Copy Markdown
Contributor Author

@iamhopaul123 iamhopaul123 Jan 20, 2023

Choose a reason for hiding this comment

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

Why is it related to the gov-cloud 🤔 Like we need to determine if they are using gov-cloud or not.

Copy link
Copy Markdown
Contributor

@efekarakus efekarakus Jan 20, 2023

Choose a reason for hiding this comment

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

I think that's my point, wouldn't moving the if-conditional above the ServiceConnectConfiguration help not have to worry about any partitions?

For all partitions there would be the same behavior of No:Value if the user doesn't have connect: true in their manifest

Copy link
Copy Markdown
Contributor Author

@iamhopaul123 iamhopaul123 Jan 20, 2023

Choose a reason for hiding this comment

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

oooh but getting rid of ServiceConnectConfiguration wouldn't help you switch from enabling service connect to disabling is the reason behind, unless you explicitly specify Enabled: False.

Copy link
Copy Markdown
Contributor

@efekarakus efekarakus Jan 20, 2023

Choose a reason for hiding this comment

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

ooh what! that's surprising to me 🤔 my guess would have been that service connect gets disabled if the config is deleted

Copy link
Copy Markdown
Contributor Author

@iamhopaul123 iamhopaul123 Jan 20, 2023

Choose a reason for hiding this comment

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

Yeah that's how ecs update-service api works :)

Copy link
Copy Markdown
Contributor

@efekarakus efekarakus Jan 20, 2023

Choose a reason for hiding this comment

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

🤦 that makes sense

@iamhopaul123 iamhopaul123 removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jan 20, 2023
@mergify mergify Bot merged commit a89fadc into aws:mainline Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dannyrandall dannyrandall Awaiting requested review from dannyrandall dannyrandall was automatically assigned from aws/aws-ecs-bdx

3 more reviewers

@efekarakus efekarakus efekarakus approved these changes

@Lou1415926 Lou1415926 Lou1415926 approved these changes

@paragbhingre paragbhingre paragbhingre approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

copilot-cli v1.24.0 causes AmazonECS ServerException when deploying

6 participants

@iamhopaul123 @codecov-commenter @efekarakus @paragbhingre @Lou1415926 @amazon-ecs-cli-v2-pr-manager