Added API definitions for Execution Time

[rockets3600]

Execution time can be passed alongside the reservation token when reserving a pass.

Added a method of setting the execution time based on plan ID.

[joel-ling]

I was about to initiate a conversation about the choice between using two google.protobuf.Timestamp fields versus a stellarstation.common.protobuf.TimeRange (defined here), and then I realised that the latter is only available internally and not part of the public API. Could @rockets3600 please confirm?

[joel-ling]

@woobianca I see in the UI design that channel set and priority level can be set when reserving a pass. Are these options not available in the API? What values would those variables take?

[joel-ling]

Okay it seems that channel set information is already represented by the reservation token (evidence).

[joel-ling]

How about priority level?

[danada]

Priority has not been included in the reservation token. We found when just adding channel sets into a reservation request made the number of tokens grow, and adding another dimension (priority) will just multiply this further.

The token is a means to ensure the user schedules only an acceptable time range and sat + antenna combo, so maybe anything outside of that should be contained within additional fields. This would mean going back to single-token reservations and adding configurable parameters (priority, channel set, execution time) into the request as plain fields (as opposed to encrypted within the token).

This is, no doubt, a fairly large change so we haven't acted on it yet. @woobianca may have more insight.

[woobianca]

It would be a large change to go back to single tokens for reservations, but it might be wise to plan for this change in this PR. I mean we should think about how we want the configurable parameters, as Dan mentioned, to be represented in the API as a whole.

Allowing priority level to be set from the API as a configurable parameter will likely be necessary in the near future.

[rockets3600]

I think that we should not change things with the anticipation of a new feature in mind. If that feature is desired in the future we can make the changes then. I want to keep things as simple as possible given the current system and the current task.

[woobianca]

The reason it might be important to think ahead is because we want to limit introducing breaking changes whenever possible.

Was wondering if you gave this example some thought:

++ message PassReservationConfiguration {
  
  google.protobuf.Timestamp execution_time_start = 1;

  google.protobuf.Timestamp execution_time_end = 2;

  [future versions]
  // Priority priority = 3;
  // string channel_set_id = 4;
}

message ReservePassRequest {
  // The token that specifies the pass, as returned in `Pass.reservation_token` or one of the
  // `Pass.channel_set_token.reservation_token` values.
  string reservation_token = 1;

  ++ PassReservationConfiguration pass_reservation_config = 2;  
}

++ message ModifyPassReservationConfigurationRequest {
  string plan_id = 1;

  PassReservationConfiguration pass_reservation_config = 2;
}

Defining the pass reservation configuration message allows the API to be very flexible with supporting new configurable fields, such as priority and channel set, while being DRY.

In addition, I like the idea of having a general ModifyPassReservationConfiguration API call instead of a specific SetPlanExecutionTime call, which would help decrease the number of requests required for making configuration changes, if both priority and execution time needed to change, for example.

Of course, it is possible to add ModifyPassReservationConfiguration later when we want to allow priority level modifications, but it may not be ideal if we want to keep our API simple, straightforward, and long-lasting.

[rockets3600]

The reason it might be important to think ahead is because we want to limit introducing breaking changes whenever possible.

I agree. If we go with either design decision we should stick to it.

Was wondering if you gave this example some thought:

Yes, I considered it. But thought that it would be simpler design for the user if the flat values exist along side the reservation_token.

Defining the pass reservation configuration message allows the API to be very flexible with supporting new configurable fields, such as priority and channel set, while being DRY.

I don't see a problem with flexibility when adding additional flat fields along side of the execution_time_start and execution_time_end. They could all be optional fields in the ReservePassRequest.

In addition, I like the idea of having a general ModifyPassReservationConfiguration API call instead of a specific SetPlanExecutionTime call, which would help decrease the number of requests required for making configuration changes, if both priority and execution time needed to change, for example.

I think a ModifyPassReservationConfiguration request which would handle updating multiple unrelated fields is a problem for separating concerns. Also I think it is more straightforward for the integrator to use atomic methods when setting the values.

[rockets3600]

I was about to initiate a conversation about the choice between using two google.protobuf.Timestamp fields versus a stellarstation.common.protobuf.TimeRange (defined here), and then I realised that the latter is only available internally and not part of the public API. Could @rockets3600 please confirm?

stellarstation.common.protobuf.TimeRange is not used anywhere in stellarstation-api repo. I believe it is exclusive to the internal repo.

[meierw]

I think I saw someone already mention the nit where the Response message should be defined after the Request message in the proto file, But that comment seems gone now, so re-introducing the idea.

[rockets3600]

That comment is in the other PR I posted! :)
But yeah, I will flip this around.

[meierw]

Is it really Required. What if the user wants to unset the value (set it to null)?

[rockets3600]

That brings up an interesting point. If the user wants to remove the PlanExecutionTime, what value would they set? Passing a null value doesn't seem proper. Should we have another method to remove the PlanExecutionTime?

The intent of this API is to set the execution time to a valid state (I don't think null should be considered valid). I argue that the two fields should be required, or else what is the point of calling it without the fields set?