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

Add application/environment to ServiceSpec generated from a Service #2988

Merged

Conversation

christophermaier
Copy link
Contributor

By not carrying this data over when generating a ServiceSpec, we
would always internally be tracking services with no
application/environment set (because it defaults to None). However,
all the spec files on disk did have this information.

This, coupled with the fact that we watch our spec directory with very
coarse event resolution (basically, if anything in the directory
changes, go check all specs to figure out what needs to be
restarted, etc.) means that anytime a new service was started or
stopped (anything that caused a change in the spec directory!), we'd
end up restarting services that had been started with
application/environment set. Our internal representation had no such
information, but the on-disk representation did have it; that's a
difference, so we restart!

Fixes #2964

Signed-off-by: Christopher Maier cmaier@chef.io

By not carrying this data over when generating a `ServiceSpec`, we
would always internally be tracking services with _no_
application/environment set (because it defaults to `None`). However,
all the spec files on disk did have this information.

This, coupled with the fact that we watch our spec directory with very
coarse event resolution (basically, if anything in the directory
changes, go check *all* specs to figure out what needs to be
restarted, etc.) means that anytime a new service was started or
stopped (anything that caused a change in the spec directory!), we'd
end up restarting services that had been started with
application/environment set. Our internal representation had no such
information, but the on-disk representation _did_ have it; that's a
difference, so we restart!

Fixes #2964

Signed-off-by: Christopher Maier <cmaier@chef.io>
@thesentinels
Copy link
Contributor

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@elliott-davis
Copy link
Contributor

tenor-54617224

@reset
Copy link
Collaborator

reset commented Aug 22, 2017

tenor-129321262

@thesentinels approve

@thesentinels
Copy link
Contributor

🤘 I am testing your branch against master before merging it. We do this to ensure that the master branch is never failing tests.

@thesentinels
Copy link
Contributor

:neckbeard: Travis CI has started testing this PR.

@thesentinels
Copy link
Contributor

💔 Travis CI reports this PR failed to pass the test suite.

The next step is to examine the job and figure out why. If it is transient, you can try re-triggering the Travis CI Job - if it passes, this PR will be automatically merged. If it is not transient, you should fix the issue and update this pull request, and issue approve again. If you believe it will never pass, and you are feeling :godmode:, you can issue a force to merge this PR anyway.

@reset
Copy link
Collaborator

reset commented Aug 22, 2017

@thesentinels approve

@thesentinels
Copy link
Contributor

🤘 I am testing your branch against master before merging it. We do this to ensure that the master branch is never failing tests.

@thesentinels
Copy link
Contributor

:neckbeard: Travis CI has started testing this PR.

@thesentinels
Copy link
Contributor

💖 Travis CI reports this PR passed.

It always makes me feel nice when humans approve of one anothers work. I'm merging this PR now.

I just want you and the contributor to answer me one question:

gif-keyboard-3280869874741411265

@thesentinels thesentinels merged commit bd9d66e into master Aug 22, 2017
@thesentinels thesentinels deleted the cm/2964-do-not-restart-all-the-things-with-app-env branch August 22, 2017 17:51
@christophermaier christophermaier added Focus:Supervisor Related to the Habitat Supervisor (core/hab-sup) component Type: Bug Issues that describe broken functionality and removed A-supervisor labels Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Focus:Supervisor Related to the Habitat Supervisor (core/hab-sup) component Type: Bug Issues that describe broken functionality
Projects
None yet
4 participants