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.

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

Fixes deadletter related tickets #220

Merged
merged 25 commits into from
Jul 4, 2018
Merged

Fixes deadletter related tickets #220

merged 25 commits into from
Jul 4, 2018

Conversation

tapaswenipathak
Copy link
Contributor

@tapaswenipathak tapaswenipathak commented Jun 18, 2018

Things related to deadletter, cli.

Closes #141.
Closes #159.
Closes #192.
Closes #193.

  • Add Tests.

cc @mapbox/platform-engine-room.

@tapaswenipathak tapaswenipathak force-pushed the watchbot branch 9 times, most recently from bc293ff to 1b6919a Compare June 18, 2018 13:53
@jakepruitt
Copy link

Just a note on file structure: I think rather than going with the watchbot v3 pattern of bin/cli.js and the command pattern like:

$ watchbot dead-letter ...

We should instead make watchbot-dead-letter a separate command that has an entry in the bin property of the package.json:

"bin": {
"watchbot": "./bin/watchbot.js",
"watchbot-log": "./bin/watchbot-log.js"
},

With "watchbot-dead-letter": "./bin/watchbot-dead-letter.js".

Then, within ./bin/watchbot-dead-letter.js, we can have all of the logic from ./lib/dead-letter.js and the meow CLI handlers.

},
"us-east-2": {
"Region": "us-east-1"
}

Choose a reason for hiding this comment

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

Why do we need to add the ECR mappings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per my understanding ECR is available in all regions but ecs-conex images are stored in few regions, would it be needed to define the mapping while creating a new ecs-watchbot service in a region as to where ecs-conex is writing images?

Choose a reason for hiding this comment

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

@tapasweni-pathak ooo good catch, I didn't check line 339 of lib/template before.

@tapaswenipathak tapaswenipathak force-pushed the watchbot branch 11 times, most recently from f264dd0 to db09a83 Compare June 26, 2018 04:00
@tapaswenipathak tapaswenipathak force-pushed the watchbot branch 2 times, most recently from c865757 to 8030079 Compare June 27, 2018 17:04
@jakepruitt
Copy link

Alright, gave a shot at refactoring some of this, but there's definitely more room for improvement if you want to pick any of this up @arunasank!

@jakepruitt
Copy link

Alright, I think all of the tests are passing now. This still needs real-life tests against dead letters with messages in them before we can merge.

@arunasank
Copy link
Contributor

arunasank commented Jul 5, 2018

👋 @jakepruitt Is there a reason you renamed the test/*.jest.js files to test/*.spec.js as part of this PR? We should update the snapshot updation script in package.json to account for this change.

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.

None yet

3 participants