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

feat(ecs): Update Task HealthStatus logic #4496

Merged
merged 2 commits into from
Apr 14, 2020
Merged

Conversation

piradeepk
Copy link
Contributor

@piradeepk piradeepk commented Apr 7, 2020

Updated the TaskHealthStatus to be determined using the HealthStatus of the task. If a health check is defined a container within the task, wait for the task health status to be healthy, otherwise, use the task status.

Addresses: spinnaker/spinnaker#5377

@piradeepk
Copy link
Contributor Author

@allisaurus @clareliguori please review

@allisaurus
Copy link
Contributor

@pkandasamy91 I think this should be "feat(ecs)" instead of "fix", since we're adding criteria to how task health is considered.

Also, I think this addresses spinnaker/spinnaker#5377 - can you mention that in the summary so it's explicit?

@piradeepk
Copy link
Contributor Author

@pkandasamy91 I think this should be "feat(ecs)" instead of "fix", since we're adding criteria to how task health is considered.

Also, I think this addresses spinnaker/spinnaker#5377 - can you mention that in the summary so it's explicit?

Will update the title. I was thinking of it along the lines of task health should have been used to determine the task status in the first place so this would be a bug (considering the task is not actually successful but Spinnaker marking it a success). Thoughts?

@allisaurus
Copy link
Contributor

allisaurus commented Apr 8, 2020

I agree it would've been preferred that health status was always taken into account, but we're now unlocking new capabilities (utilizing container health) and also changing the behavior of a core competency (evaluating task health -> deployment success/fail).

Put another way: the behavior change is enough that I would vote against patching this to previous versions. Feels like a minor release kind of change re: expectations, etc. What do you think?

@clareliguori
Copy link
Member

Agree on this being a feat

@piradeepk
Copy link
Contributor Author

piradeepk commented Apr 8, 2020

Put another way: the behavior change is enough that I would vote against patching this to previous versions. Feels like a minor release kind of change re: expectations, etc. What do you think?

I definitely see how it can be both a fix and a feat. I agree, we should not patch this to previous versions, as this could potentially break existing customers, and the previous behaviour might have been relied upon by customers. I'll update the pr title to be a feature.

@piradeepk piradeepk changed the title fix(ecs): Update Task HealthStatus logic feat(ecs): Update Task HealthStatus logic Apr 8, 2020
@piradeepk piradeepk force-pushed the master branch 2 times, most recently from 5b35f29 to a97dd82 Compare April 8, 2020 17:57
@piradeepk piradeepk force-pushed the master branch 2 times, most recently from cdca393 to b6bca29 Compare April 9, 2020 00:39
@piradeepk
Copy link
Contributor Author

Feedback addressed :)

@clareliguori clareliguori added the ready to merge Approved and ready for a merge label Apr 14, 2020
@mergify mergify bot added the auto merged Merged automatically by a bot label Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot ready to merge Approved and ready for a merge target-release/1.20
Projects
None yet
4 participants