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 Sundog to handle dynamic settings #49

Merged
merged 14 commits into from Jul 9, 2019
Merged

Add Sundog to handle dynamic settings #49

merged 14 commits into from Jul 9, 2019

Conversation

zmrow
Copy link
Contributor

@zmrow zmrow commented Jun 28, 2019

Sundog is a small tool to allow for settings that must be gathered
and set at runtime. It requests the generators that must be run
from the API and runs them. Using that output, it attempts to
set them using the API.

And yes... we really need an API client. :)

This builds on the changes made in #48

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

sundog is a small tool to allow for settings that must be gathered
and set and runtime. It requests the generators that must be run
from the API and runs them. Using that output, it attempts to
set them using the API.
@zmrow zmrow self-assigned this Jun 28, 2019
@zmrow zmrow requested review from tjkirch and bcressey June 28, 2019 20:43
Copy link
Contributor

@tjkirch tjkirch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind pulling #48 and trying this against the API changes?

workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
zmrow and others added 3 commits July 3, 2019 09:31
Co-Authored-By: Tom <tjkirch@users.noreply.github.com>
Co-Authored-By: Tom <tjkirch@users.noreply.github.com>
Co-Authored-By: Tom <tjkirch@users.noreply.github.com>
// The API takes a properly nested Settings struct, so deserialize our map to a Settings
// and ensure it is correct
let settings_struct: model::Settings =
deserialization::from_map(&setting_map).context(error::Deserialize)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We tested together and found an issue here. from_map is a datastore-level construct, which means it assumes you're serializing/deserializing as necessary. That's not fair for users outside of the API server, who shouldn't have to know anything about the ser/de going on inside. I'm planning on making a higher-level from_map-type function that's pub, and that handles the ser/de internally. I hope to have that within a (working) day or two so you can update this with minimal changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After further thought, I'm not sure what I wanted is possible.

Important background:

  • You can't have mixed value types in a collection like a HashMap, they must be homogeneous
  • Settings (for example) can't be assumed to have all String values, we want to support any serializable type

Because of this, the options for deserializing from a collection are:

  • Deserializing from a (homogeneous) collection with a subset of the keys that have the same type, which only works if all other fields are optional, as they are with Settings. (This would technically work for sundog's use case, but I think it's a bad and confusing interface.)
  • Treat the String as a serialized structure, allowing you to obviate the homogeneous type requirement by using the serialization format's type system instead, hidden in a String

Note: You can't have the values be serde's Serialize type because it's not object safe -- this means you can't have a collection of Box<Serialize>, for example, or accept a HashMap<String, Serialize>; all things must be the same type that implements Serialize.

So, unfortunately, I'm not aware of a way to make a higher-level interface for something like from_map that completely hides the serialization requirement. I did make serialization helpers so you don't have to care about the exact serialization format - we use JSON, but could switch.

@zmrow - I think the right answer is for you to call datastore::serialize_scalar() on the output of the generators and store the resulting string in your HashMap; then you can from_map that into a Settings as you are now.

// datastore-level construct `from_map` on it. `from_map` treats
// strings as a serialized structure.
let serialized_output = datastore::serialize_scalar(&output)
.context(error::SerializeScalar { output: output })?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General Rust nit: if the field name is the same as the variable, you don't need to repeat it, e.g. "{ output }" is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, I knew that.

workspaces/api/sundog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/sundog/src/main.rs Show resolved Hide resolved
@zmrow zmrow merged commit 0334516 into develop Jul 9, 2019
@zmrow zmrow deleted the sundog-poc branch July 9, 2019 16:53
@zmrow
Copy link
Contributor Author

zmrow commented Jul 17, 2019

This work is part of #22. Just cross-referencing here...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants