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

OBS-393 Simplify user interface for ECS installation #237

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

OBS-393 Simplify user interface for ECS installation #237

wants to merge 8 commits into from

Conversation

gmann42
Copy link
Contributor

@gmann42 gmann42 commented Sep 25, 2023 •
edited

Why ?

What

  • Removes interactiveness from ECS cli
    New Usage
POSTMAN_API_KEY=<postman-api-key> postman-lc-agent ecs add \
--collection <postman-collectionID> \
--region <aws-region> \
--cluster <full ARN of ECS cluster> \
--service <full ARN of ECS service>
  • Deletes un-used code listECSClusters, listTasks, ListServices etc.
  • Adds README.md for usage.

How

  • User inputs aws region, full ARN of cluster and service
  • Load profile AWS credentials
  • DescribeCluster to verify existence and get cluster name
  • DescribeService to verify existence and get TaskDefinitonARN
  • TaskDefinitonARN fetched is then used to describeTask
  • modifyTask use RegisterTask to create a new definition with postman-lc-agent container as the side car
  • updateService udpates the ECS service with new task definition

Ticket link: https://postmanlabs.atlassian.net/browse/OBS-393

mgritter
mgritter reviewed Sep 26, 2023
View reviewed changes
cmd/internal/ecs/add.go Outdated Show resolved Hide resolved
cmd/internal/ecs/add.go Show resolved Hide resolved
cmd/internal/ecs/add.go Outdated Show resolved Hide resolved
cmd/internal/ecs/add.go Outdated Show resolved Hide resolved
cmd/internal/ecs/add.go Show resolved Hide resolved
cmd/internal/ecs/add.go

wf.ecsService = aws.ToString(service.ServiceName)
wf.ecsServiceARN = arn(ecsServiceFlag)
wf.ecsTaskDefinitionARN = arn(*service.TaskDefinition)
Copy link
Contributor

Choose a reason for hiding this comment

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

Dereference without checking nil, what if there is no task registered yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A service can not be created without a task, we are fetching the service from AWS and using it's output to pick the taskARN

cmd/internal/ecs/add.go Outdated Show resolved Hide resolved
cmd/internal/ecs/add.go Outdated
Comment on lines 671 to 320
printer.Infof("Create a new version %d of task definition %q which includes the Postman Live Collections Agent as a sidecar.\n",
printer.Infof("Create a new version of task definition %q which includes the Postman Live Collections Agent as a sidecar.\n",
wf.ecsTaskDefinition.Revision+1, wf.ecsTaskDefinitionFamily)
Copy link
Contributor

Choose a reason for hiding this comment

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

You removed %d but kept it in the parameter list. We should specify the version number if it is still possible to do so.

Copy link
Contributor Author

@gmann42 gmann42 Sep 26, 2023 •
edited

Choose a reason for hiding this comment

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

AWS auto-increments the revision numbers, we can't just do current + 1.

Revisions    Status
1                    Active
2                    Active
3                   Deployed in service
4                   Active
5                   Inactive(or even deleted aws remembers the highest count always)

New task definition revision will be 6, +1 would show incorrect value of 4.
For my testing i had deleted or marked inactive bunch of times, it always remembers the last highest + 1.
So this is not reliable without some sort of hack

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, good point. Do we show the revision number of our modified task anywhere in our output?

cmd/internal/ecs/ecs.go Outdated Show resolved Hide resolved
cmd/internal/ecs/ecs.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

None yet
3 participants
@gmann42 @mgritter @nerminamiller-postman