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

Evolve the supervisor's HTTP API #5787

Merged
merged 9 commits into from Oct 31, 2018

Conversation

Projects
None yet
5 participants
@raskchanky
Member

raskchanky commented Oct 25, 2018

This PR is meant to do two things:

  1. Deprecate parts of the supervisor's API
  2. Implement replacements for the deprecated parts

I haven't actually removed anything at this point. All the old baggage is still there, though marked as $deprecated in our JSON schema. The new replacements are along side the old.

In the case of the /services and /census endpoints, most of the changes are just small tweaks to what we already have.

The /butterfly endpoint was different though. In that case, I deprecated all of the top level keys and implemented the replacements with all new top level keys. The reason for this is due to the fact that serialize_struct, from serde, requires keys with the 'static lifetime. In many cases, we're dealing with data that's coming from a container like HashMap, where we don't know the key names up front. The key names are all Strings but you can't convert a String into a 'static &str unless you use Box::leak() and intentionally leak the memory (at least that I could find).

In the end, it was dramatically easier to just create new top level keys that looked the way I wanted them to, rather than trying to shoehorn the representation we wanted into the existing top level keys. This is one area where Rust the language hamstrung us a bit. In a garbage collected language, this would've been a non-issue.

Closes #5692

@thesentinels

This comment has been minimized.

Contributor

thesentinels commented Oct 25, 2018

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!

@raskchanky raskchanky force-pushed the jb/evolve-http-api branch from e41534f to acf10c4 Oct 25, 2018

@baumanj

This comment has been minimized.

Member

baumanj commented Oct 27, 2018

Will try to get to looking at this on Monday

@raskchanky raskchanky force-pushed the jb/evolve-http-api branch from acf10c4 to 1e53f63 Oct 29, 2018

@baumanj

This comment has been minimized.

Member

baumanj commented Oct 29, 2018

You'll probably want to do a find/replace for "Since 0.66.0" before this merges, since it looks like it'll be coming after 0.67 at least.

@christophermaier

This looks great overall, but I think the lowercasing of various enum members should be removed for now (see comment for more).

@@ -1097,6 +1097,7 @@ impl<'a> Serialize for ServiceProxy<'a> {
strukt.serialize_field("smoke_check", &s.smoke_check)?;
strukt.serialize_field("spec_file", &s.spec_file)?;
strukt.serialize_field("spec_ident", &s.spec_ident)?;
strukt.serialize_field("spec_identifier", &s.spec_ident.to_string())?;

This comment has been minimized.

@christophermaier

christophermaier Oct 29, 2018

Contributor

Might want to make a note on the package identifier type to re-work its Serialize/Deserialize implementations to return a string at some point in the future.

(We can't actually change it right now, though, since that would be undoing all the work you've done to make the deprecation painless!)

cc: @fnichol, since I believe he has something along these lines in the works.

This comment has been minimized.

@raskchanky

raskchanky Oct 30, 2018

Member

Will do

strukt.serialize_field("member_id", &self.0.member_id)?;
strukt.serialize_field("pkg", &self.0.pkg)?;
if let Some(ref p) = self.0.pkg {

This comment has been minimized.

@christophermaier

christophermaier Oct 29, 2018

Contributor

When would this ever be None? 🤔

This comment has been minimized.

@raskchanky

raskchanky Oct 30, 2018

Member

The pkg field on CensusMember is optional, hence this code.

This comment has been minimized.

@christophermaier

christophermaier Oct 30, 2018

Contributor

🤔 That is unexpected... this serialization code is fine, of course, but it seems odd that we'd ever be in a situation where a census member isn't running a package.

@@ -310,7 +310,6 @@ impl From<ElectionStatusRumor> for ElectionStatus {
pub struct ServiceFile {
pub filename: String,
pub incarnation: u64,
#[serde(skip_serializing)]

This comment has been minimized.

@christophermaier

christophermaier Oct 29, 2018

Contributor

How were we returning a byte array if we were skipping the serialization?

use serde::{
de,
ser::{SerializeMap, SerializeStruct},
Deserialize, Deserializer, {Serialize, Serializer},

This comment has been minimized.

@christophermaier

christophermaier Oct 29, 2018

Contributor

Why are Serialize and Serializer in braces, but the De* objects aren't?

This comment has been minimized.

@raskchanky

raskchanky Oct 30, 2018

Member

No idea, honestly. I'll fix it.

strukt.serialize_field("incarnation", &self.0.incarnation)?;
strukt.serialize_field("persistent", &self.0.persistent)?;
strukt.serialize_field("swim_port", &self.0.swim_port)?;
strukt.serialize_field("health", &self.1)?;

This comment has been minimized.

@christophermaier

christophermaier Oct 29, 2018

Contributor

This is such a better representation.

let mut new_map = HashMap::new();
for (k, v) in map.iter() {
let election = v.get("service_config").clone();

This comment has been minimized.

@christophermaier

christophermaier Oct 29, 2018

Contributor

Copy-paste error here with election?

This comment has been minimized.

@raskchanky

raskchanky Oct 30, 2018

Member

Yep, my bad

"b8f013535b3d4c85822e442f0c12801e": "alive",
"be57ce74c75d4b29a4d8602c28397364": "alive",
"c2ca91559a1f4114a819a1ca283cf10a": "alive",
"f723be0d353e45c7bfa097aa586b8795": "alive"

This comment has been minimized.

@christophermaier

christophermaier Oct 29, 2018

Contributor

This change is actually going to be a breaking one... I imagine folks that are using the HTTP gateway to do real work are doing things like filtering on these various states, and their filters are probably case-sensitive.

We should hold this commit, and advise users to have their filters be case insensitive. Then after a few more releases, we can add this in.

This comment has been minimized.

@raskchanky

raskchanky Oct 30, 2018

Member

No problem, I'll revert

@raskchanky

This comment has been minimized.

Member

raskchanky commented Oct 30, 2018

@christophermaier Your feedback has been addressed.

raskchanky added some commits Oct 19, 2018

Deprecate fields in /service output and implement replacements
Signed-off-by: Josh Black <raskchanky@gmail.com>
Deprecate fields in /census output and implement replacements
Signed-off-by: Josh Black <raskchanky@gmail.com>
Fix test breakage
Signed-off-by: Josh Black <raskchanky@gmail.com>
Correct the definition of a service file, and deprecate 'body'
Signed-off-by: Josh Black <raskchanky@gmail.com>
Deprecate fields in /butterfly output and implement replacements
Signed-off-by: Josh Black <raskchanky@gmail.com>
Add some explanatory comments
Signed-off-by: Josh Black <raskchanky@gmail.com>
Unify Display implementations to be lowercase
Signed-off-by: Josh Black <raskchanky@gmail.com>
Address PR feedback
Signed-off-by: Josh Black <raskchanky@gmail.com>

@raskchanky raskchanky force-pushed the jb/evolve-http-api branch from 701207a to e6cccc9 Oct 30, 2018

@christophermaier

This comment has been minimized.

Contributor

christophermaier commented Oct 31, 2018

@raskchanky Why not just revert (or rather, remove) 3cfc28b? Looks like there are some changes in that commit that are still present.

@raskchanky

This comment has been minimized.

Member

raskchanky commented Oct 31, 2018

@christophermaier Sorry, I misunderstood your initial feedback, thinking that only the changes to Health were problematic. I'll revert that commit.

Revert "Unify Display implementations to be lowercase"
This reverts commit 3cfc28b.

Signed-off-by: Josh Black <raskchanky@gmail.com>

@raskchanky raskchanky force-pushed the jb/evolve-http-api branch from d3a3d4e to 0eada73 Oct 31, 2018

@bixu

This comment has been minimized.

Contributor

bixu commented Oct 31, 2018

2 things:

1). I'm excited to take advantage of health information in the Supervisor API.
2). It's a pleasure and also an education to read the Habitat team's code reviews. Thanks for letting the public look at this!

Comments were addressed

@raskchanky raskchanky merged commit 489ac97 into master Oct 31, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
expeditor/config-validation Validated your Expeditor config file
Details

@raskchanky raskchanky deleted the jb/evolve-http-api branch Oct 31, 2018

chef-ci added a commit that referenced this pull request Oct 31, 2018

Update CHANGELOG.md with details from pull request #5787
Obvious fix; these changes are the result of automation not creative thinking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment