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

Fix config gossip #6910

Merged
merged 1 commit into from Sep 4, 2019

Conversation

@davidMcneil
Copy link
Contributor

commented Aug 28, 2019

Resolves #6907

Signed-off-by: David McNeil mcneil.david2@gmail.com

@chef-expeditor

This comment has been minimized.

Copy link

commented Aug 28, 2019

Hello davidMcneil! 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!

let ctx = self.render_context(census_ring);
TemplateUpdate::new(self.compile_hooks(&ctx),
self.compile_configuration(&ctx),
self.hooks.reconfigure.is_some() || self.hooks.reload.is_some())
} else {
TemplateUpdate::default()
}
};
(template_data_changed, template_update)

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 28, 2019

Contributor

I haven't gotten all the way through my review / validation, but the addition of an extra bool here, while perhaps technically correct and expedient, feels a bit like treating a symptom rather than a cause. While we're digging into this code, I'd like to try and get at the underlying complexity that's here and simplify it, or at least very thoroughly document it and codify it into types when possible (e.g., can this bool, if it is indeed necessary, be folded in to the TemplateUpdate type, either as a field or as a method?).

This comment has been minimized.

Copy link
@davidMcneil

davidMcneil Aug 28, 2019

Author Contributor

Completely agree. I would really like to understand the why. I think the question comes down to what should the return value of tick mean? Or said another way, when should it return true and when should it return false? This is how we ultimately decide to send out the latest gossip here.

@davidMcneil davidMcneil self-assigned this Aug 28, 2019
@mwrock
mwrock approved these changes Aug 28, 2019
Copy link
Contributor

left a comment

This looks good to me for unblocking the release. I think @christophermaier 's comment is great but IMO could come in a subsequent PR

Copy link
Contributor

left a comment

Is #6907 a regression? Do we know the change that caused it? The logic here seems reasonable, but I'm having a harder time understanding how we got to the defect behavior in the first place, which makes me nervous about signing off on a fix.

}
}
}
}
};

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 28, 2019

Contributor

Why have a semicolon here? That usually indicates that the value of a match expression is being bound with a let statement, but that's not the case here, so it makes me wonder if I'm missing something.

This comment has been minimized.

Copy link
@davidMcneil

davidMcneil Aug 29, 2019

Author Contributor

I had it returning a value at one point, and it just did not get removed. 🤔 It seems like Clippy should be able to catch that.

@@ -739,7 +739,7 @@ impl Service {

/// Compares the current state of the service to the current state of the census ring and the
/// user-config, and re-renders all templatable content to disk.
fn update_templates(&mut self, census_ring: &CensusRing) -> TemplateUpdate {
fn update_templates(&mut self, census_ring: &CensusRing) -> (bool, TemplateUpdate) {

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 28, 2019

Contributor

Can we use this opportunity to switch this (and ideally the return of tick) from a bool to an enum that would be more illustrative?

This comment has been minimized.

Copy link
@davidMcneil

davidMcneil Aug 29, 2019

Author Contributor

Agreed, this is one of the still open questions: what should tick return and therefore what should update_templates return?

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 29, 2019

Contributor

Totally agree about using an enum here... I'm digging into the question of tick right now.

@davidMcneil

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

Is #6907 a regression? Do we know the change that caused it?

Yes, #6668

This PR restores the functionality as it was before #6668, but we would like to understand why this behavior is correct.

@dmccown

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

Closing this PR in favor of a fix @christophermaier is working on.

@dmccown dmccown closed this Sep 3, 2019
This is a temporary fix that should be reverted when
the correct solution is in place.

Signed-off-by: David McNeil <mcneil.david2@gmail.com>
@davidMcneil

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

Reopening to provide a temporary fix.

@davidMcneil davidMcneil reopened this Sep 4, 2019
@davidMcneil davidMcneil force-pushed the dmcneil/fix-config-gossip branch from 09c2615 to 1dd56be Sep 4, 2019
Copy link
Contributor

left a comment

This should recover the desired functionality, but it is not the final fix. I'm working on a more in-depth refactoring to address this, but it's taken me to some unexpected places, so we're resurrecting this fix.

@davidMcneil davidMcneil merged commit 7ae4012 into master Sep 4, 2019
5 checks passed
5 checks passed
DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #3316 passed (20 minutes, 5 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #435 passed (45 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details
@chef-ci chef-ci deleted the dmcneil/fix-config-gossip branch Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.