Support injecting property values into entry point command.

[chrisstaite-menlo]

Resolves #261

---

This change is 

[chrisstaite]

I'm thinking of making this a breaking change so entrypoint_cmd is a list

[allada]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @chrisstaite-menlo)

a discussion (no related file):
I think we should do this by injecting them in as environmental variables.

To prevent it from being a conflict, lets have the user specify the environmental variable they'd like to assign the properties too in the config.

I'm leaning towards a map, where the user specifies each environ what platform property to assign to it, as it would make it much easier to use.

---

---

cas/worker/running_actions_manager.rs line 559 at r1 (raw file):

    fn inject_property<'a>(&'a self, argument: &'a str) -> &'a str {
        if argument.starts_with('{') && argument.ends_with('}') {

nit: I'd rewrite this:

if !argument.starts_with('{') || !argument.ends_with('}') {
  return argument;
}
let property_name = &argument[1..argument.len() - 1];
match self.action_info.platform_properties.properties.get(property_name) {
  Some(PlatformPropertyValue::Exact(value)) => value.as_str(),
  // TODO: Support injecting minimum.
  Some(PlatformPropertyValue::Minimum(_)) => argument,
  Some(PlatformPropertyValue::Priority(value)) => value.as_str(),
  Some(PlatformPropertyValue::Unknown(value)) => value.as_str(),
  None => argument,
}

---

cas/worker/running_actions_manager.rs line 560 at r1 (raw file):

    fn inject_property<'a>(&'a self, argument: &'a str) -> &'a str {
        if argument.starts_with('{') && argument.ends_with('}') {
            // Inject property

nit: needless comment.

[chrisstaite-menlo]

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @allada)

a discussion (no related file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

I think we should do this by injecting them in as environmental variables.

To prevent it from being a conflict, lets have the user specify the environmental variable they'd like to assign the properties too in the config.

I'm leaning towards a map, where the user specifies each environ what platform property to assign to it, as it would make it much easier to use.

Done.

---

[allada]

Reviewed 1 of 5 files at r1, 2 of 5 files at r2, all commit messages.
Reviewable status: 3 of 7 files reviewed, 1 unresolved discussion (waiting on @chrisstaite-menlo)

---

config/cas_server.rs line 325 at r2 (raw file):

    /// the job with the value of the environment variable being the value of
    /// the property of the action being executed of that name.
    pub property_environments: Option<HashMap<String, String>>,

I think this logic is nearly identical to also supporting: #237

Can we do a few minor tweaks to this so we can pass in raw (static) values as well as dynamic values like platform properties?

Introducing a new enum is probably best and have the key be the source type of the string (ie: Raw(String) or PlatformPropertyValue(String)). This might be useful for other weird use cases in the future for metadata injection.

[chrisstaite-menlo]

Reviewable status: 1 of 7 files reviewed, 1 unresolved discussion (waiting on @allada)

---

config/cas_server.rs line 325 at r2 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

I think this logic is nearly identical to also supporting: #237

Can we do a few minor tweaks to this so we can pass in raw (static) values as well as dynamic values like platform properties?

Introducing a new enum is probably best and have the key be the source type of the string (ie: Raw(String) or PlatformPropertyValue(String)). This might be useful for other weird use cases in the future for metadata injection.

I also have a use for this with the reclient stuff.

[allada]

Reviewed 1 of 5 files at r1, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @chrisstaite-menlo)

---

cas/worker/running_actions_manager.rs line 606 at r3 (raw file):

                let value = match source {
                    EnvironmentSource::Property(property) => {
                        match self.action_info.platform_properties.properties.get(property) {

nit: Can we implement ToString for PlatformPropertyValue then just call .to_string() here?

---

cas/worker/running_actions_manager.rs line 607 at r3 (raw file):

                    EnvironmentSource::Property(property) => {
                        match self.action_info.platform_properties.properties.get(property) {
                            // Only set in Unknown in the worker -

This doesn't seem very future proof. I think we should pass all of them even though we only support Unknown here. This way if we ever "fix" the properties to be known it won't silently break here.

---

cas/worker/running_actions_manager.rs line 1079 at r3 (raw file):

#[derive(Default)]
pub struct ExecutionConfiguration {

nit: I try to ensure all public structs and properties (that are pub as well) are documented.

Can we add a few doc comments here of what this is and it's use?

---

cas/worker/tests/running_actions_manager_test.rs line 141 at r3 (raw file):

mod running_actions_manager_tests {
    use super::*;
    use config::cas_server::EnvironmentSource;

nit, please put this in the top use section.

---

cas/worker/tests/running_actions_manager_test.rs line 1219 at r3 (raw file):

            let test_wrapper_script = OsString::from(test_wrapper_dir + "\\test_wrapper_script.bat");
            let mut test_wrapper_script_handle = fs::create_file(&test_wrapper_script).await?;
            test_wrapper_script_handle

It turns out we need to .flush() here before we drop the file.

Example: https://github.com/allada/turbo-cache/pull/274/files#diff-88dd8296b1ee148349ea178051ddb0ee927b388cc9a20fdd3979bf0ff9e60895R1116

---

config/cas_server.rs line 254 at r3 (raw file):

    /// The name of the property in the action to get the value from.
    Property(String),
    /// The value to set.

nit: The raw value to set.

---

config/cas_server.rs line 328 at r3 (raw file):

    pub precondition_script: Option<String>,

    /// An optional mapping of environment names to set for the execution

This comment looks to be related specifically to platform properties. Can we amend it to be generic, then put the platform property specific stuff in the EnvironmentSource::Property comment?

[chrisstaite-menlo]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @allada)

---

cas/worker/running_actions_manager.rs line 606 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: Can we implement ToString for PlatformPropertyValue then just call .to_string() here?

That would make a needless copy though as we only need a &str.

---

cas/worker/running_actions_manager.rs line 607 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

This doesn't seem very future proof. I think we should pass all of them even though we only support Unknown here. This way if we ever "fix" the properties to be known it won't silently break here.

I'm not sure how we could fix it since it comes from the Protobuf that has no knowledge of the scheduler.

---

cas/worker/tests/running_actions_manager_test.rs line 141 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit, please put this in the top use section.

Editor...

---

config/cas_server.rs line 328 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

This comment looks to be related specifically to platform properties. Can we amend it to be generic, then put the platform property specific stuff in the EnvironmentSource::Property comment?

It is generic, I re-worded it specifically for that purpose.

[allada]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @chrisstaite-menlo)

---

cas/worker/running_actions_manager.rs line 606 at r3 (raw file):

Previously, chrisstaite-menlo (Chris Staite) wrote…

That would make a needless copy though as we only need a &str.

How about .to_str() this would do the same, but without a copy.

---

cas/worker/running_actions_manager.rs line 607 at r3 (raw file):

Previously, chrisstaite-menlo (Chris Staite) wrote…

I'm not sure how we could fix it since it comes from the Protobuf that has no knowledge of the scheduler.

It's more about ensuring intended behavior. Maybe one example is if we modify the proto messages to encode this information for inter-system communications and this value no longer publishes. This would be a silent failure. Adding the various properties seems feels like it would be trivial to maintain (or if we do it via .to_str() it won't be an issue).

[chrisstaite-menlo]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @allada)

---

cas/worker/running_actions_manager.rs line 606 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

How about .to_str() this would do the same, but without a copy.

Can't be implemented for Minimum.

[allada]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @chrisstaite-menlo)

---

cas/worker/running_actions_manager.rs line 606 at r3 (raw file):

Previously, chrisstaite-menlo (Chris Staite) wrote…

Can't be implemented for Minimum.

Hmmm, Maybe use Cow<'a, str>?

This way only Minimum does an allocation?

[chrisstaite-menlo]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @allada)

---

cas/worker/running_actions_manager.rs line 607 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

It's more about ensuring intended behavior. Maybe one example is if we modify the proto messages to encode this information for inter-system communications and this value no longer publishes. This would be a silent failure. Adding the various properties seems feels like it would be trivial to maintain (or if we do it via .to_str() it won't be an issue).

Done.

---

cas/worker/running_actions_manager.rs line 1079 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: I try to ensure all public structs and properties (that are pub as well) are documented.

Can we add a few doc comments here of what this is and it's use?

That's missing for Callbacks 😉

---

cas/worker/tests/running_actions_manager_test.rs line 1219 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

It turns out we need to .flush() here before we drop the file.

Example: https://github.com/allada/turbo-cache/pull/274/files#diff-88dd8296b1ee148349ea178051ddb0ee927b388cc9a20fdd3979bf0ff9e60895R1116

Done.

[allada]

Reviewed 8 of 8 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @chrisstaite-menlo)

---

cas/worker/running_actions_manager.rs line 1079 at r3 (raw file):

Previously, chrisstaite-menlo (Chris Staite) wrote…

That's missing for Callbacks 😉

"try" 😆

---

cas/worker/running_actions_manager.rs line 606 at r4 (raw file):

                let value = match source {
                    EnvironmentSource::Property(property) => {
                        if let Some(property) = self.action_info.platform_properties.properties.get(property) {

fyi this might look better:

self
    .action_info
    .platform_properties
    .properties
    .get(property)
    .map_or_else(|| Cow::Borrowed(""), |v| v.to_str())

[allada]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @chrisstaite-menlo)

[chrisstaite-menlo]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @chrisstaite-menlo)

---

cas/worker/running_actions_manager.rs line 606 at r4 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

fyi this might look better:

self
    .action_info
    .platform_properties
    .properties
    .get(property)
    .map_or_else(|| Cow::Borrowed(""), |v| v.to_str())

Certainly does.

[allada]

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @chrisstaite-menlo)

[allada]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @chrisstaite-menlo)