search

awslabs / amazon-app-runner-deploy

Registers an AWS AppRunner Service and deploys the application using the source code of a given GitHub repository. Supports both source code and Docker image based service
MIT No Attribution
50 stars 32 forks source link

Add possibility to configure health checks #7

Closed PatrickGotthard closed 2 years ago

PatrickGotthard commented 2 years ago

Adds support to configure health checks. Is not limited to HTTP-based checks and solves #5

hariohmprasath commented 2 years ago

Hi @PatrickGotthard, Thanks for the contribution. I did a quick review of your code looks good to me. Can you add a couple of test cases to index.test.ts?

@DmitryGulin, Can you do a quick review as well?

DmitryGulin commented 2 years ago

Agree with @hariohmprasath , let's have the new functionality covered with tests first.

PatrickGotthard commented 2 years ago

Hi @DmitryGulin, @hariohmprasath,

I'll add tests as soon as possible. I looked at the existing tests and it should be not that hard - even though I'm no JavaScript expert. But it would be much easier to write tests for changes like this when the config and deploy parts would be decoupled from each other - but that requires a major rewrite of the code.

Regards, Patrick

PatrickGotthard commented 2 years ago

Hi @DmitryGulin, @hariohmprasath,

in the meantime I rewrote the action from scratch because I found the code hard to read/maintain and we also had to add support for autoscaling while also not being interested in code-based deployments. Do you want to fork this pull-request or should I just close this pull-request?

Regards, Patrick

hariohmprasath commented 2 years ago

Hi @PatrickGotthard, If the code is backward compatible, why not to contribute back to this repository? Sounds interesting and would love to review it and take it in. Thanks

PatrickGotthard commented 2 years ago

Hi @hariohmprasath,

the code is not backwards compatible, I rewrote everything from scratch without code-based deployment support.

Regards, Patrick

DmitryGulin commented 2 years ago

Hi @PatrickGotthard , If your new code includes functionality from the first PR, I'd recommend closing the first one and creating a new PR.

PatrickGotthard commented 2 years ago

Hi @DmitryGulin, @hariohmprasath,

my new implementation has nothing to do with yours - I created a new action completely from scratch. I just wanted to ask you if you want to create a branch of my pull requests and add the missing tests yourself. Otherwise I'll delete my fork and everything is gone.

Regards, Patrick

hariohmprasath commented 2 years ago

Thanks for the response @PatrickGotthard, let's close this pull request and when you get a chance you can work on making your code backward compatible and submit a new PR