Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: update app show to display if workload is deployed #3379

Merged
merged 16 commits into from
Mar 30, 2022
Merged

feat: update app show to display if workload is deployed #3379

merged 16 commits into from
Mar 30, 2022

Conversation

paragbhingre
Copy link
Contributor

This PR adds new environments column to the workloads table in app show command displaying the environments to which workloads are deployed to.

This PR fixes the issue #3366

@paragbhingre paragbhingre requested a review from a team as a code owner March 17, 2022 20:21
@paragbhingre paragbhingre requested review from iamhopaul123 and removed request for a team March 17, 2022 20:21
@amazon-ecs-cli-v2-pr-manager amazon-ecs-cli-v2-pr-manager requested review from Lou1415926 and removed request for iamhopaul123 March 17, 2022 20:21
@paragbhingre
Copy link
Contributor Author

Please ignore test failures and observe the commented code as I have open this PR for your 1st glance so that we can decide which path to choose to display environments in workloads table. Please let me know your thoughts on the implementation. :)

@huanjani
Copy link
Contributor

Please ignore test failures and observe the commented code as I have open this PR for your 1st glance so that we can decide which path to choose to display environments in workloads table. Please let me know your thoughts on the implementation. :)

I like the first option! 😄

Lou1415926
Lou1415926 reviewed Mar 18, 2022
View reviewed changes
Lou1415926
Lou1415926 reviewed Mar 22, 2022
View reviewed changes
Copy link
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.

LGTM! Thank you for the unit tests!

I have mostly just nits and a suggestion/though on Environments column when the service isn't deployed at all.

Also I think we should probably consider moving description from app_show.go to describe/app.go. Not the scope of this PR though, just a call out!

iamhopaul123
iamhopaul123 reviewed Mar 22, 2022
View reviewed changes
dannyrandall
dannyrandall reviewed Mar 23, 2022
View reviewed changes
Copy link
Contributor

@dannyrandall dannyrandall left a comment

Choose a reason for hiding this comment

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

love the errgroup! awesome perf improvement😊

iamhopaul123
iamhopaul123 reviewed Mar 23, 2022
View reviewed changes
Copy link
Contributor

@iamhopaul123 iamhopaul123 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.

Nice work for improving the performance! Thanks for diving deep into it. +1 to Danny's comments.

efekarakus
efekarakus reviewed Mar 23, 2022
View reviewed changes
efekarakus
efekarakus reviewed Mar 28, 2022
View reviewed changes
Copy link
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.

Looks good! few small changes

efekarakus
efekarakus approved these changes Mar 30, 2022
View reviewed changes
@efekarakus efekarakus added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Mar 30, 2022
@efekarakus efekarakus changed the title chore(app): update app show to display if workload is deployed feat(app): update app show to display if workload is deployed Mar 30, 2022
@efekarakus efekarakus changed the title feat(app): update app show to display if workload is deployed feat: update app show to display if workload is deployed Mar 30, 2022
dannyrandall
dannyrandall approved these changes Mar 30, 2022
View reviewed changes
Copy link
Contributor

@dannyrandall dannyrandall left a comment

Choose a reason for hiding this comment

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

lgtm! great job🎉

@paragbhingre paragbhingre removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Mar 30, 2022
@mergify mergify bot merged commit 8e5a9b7 into aws:mainline Mar 30, 2022
@paragbhingre paragbhingre deleted the displayDeployedWorkloadInAppShow branch January 26, 2023 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dannyrandall dannyrandall dannyrandall approved these changes

@iamhopaul123 iamhopaul123 iamhopaul123 left review comments

@huanjani huanjani huanjani left review comments

@Lou1415926 Lou1415926 Lou1415926 left review comments

@efekarakus efekarakus efekarakus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@paragbhingre @huanjani @efekarakus @dannyrandall @iamhopaul123 @Lou1415926