Support remote download of files when doing a File.Get #11
Conversation
file/file.proto
Outdated
| @@ -18,6 +18,7 @@ syntax = "proto3"; | |||
|
|
|||
| package gnoi.file; | |||
|
|
|||
| import "third_party/openconfig/gnoi/common.proto"; | |||
There was a problem hiding this comment.
Please correct this import path :-)
file/file.proto
Outdated
| // where to transfer the data. The remote_file must be an absolute path to a | ||
| // file. | ||
| message GetRemoteRequest { | ||
| string remote_file = 1; |
There was a problem hiding this comment.
How does this remote_file tally with the path in the RemoteDownload message?
I'd have thought that the required field in this message would be the local file to which the remote file is being downloaded?
There was a problem hiding this comment.
Yea, agree this is a bit confusing and the docs don't help :)
The intention was RemoteDownload specified how to get the file, and this was supposed to specify the path name in the device to save it to (i.e. /tmp/os-1.0).
Maybe local_file be a better name and I can update the descriptions as well. Please let me know what you think.
There was a problem hiding this comment.
Yep - something like local_path would probably make more sense here IMHO.
system/system.proto
Outdated
| @@ -24,6 +24,7 @@ syntax = "proto3"; | |||
|
|
|||
| package gnoi.system; | |||
|
|
|||
| import "third_party/openconfig/gnoi/common.proto"; | |||
common.proto
Outdated
|
|
||
| package gnoi; | ||
|
|
||
| import "third_party/openconfig/gnoi/types.proto"; |
| @@ -28,6 +29,13 @@ service File { | |||
| // if the file does not exist or there was an error reading the file. | |||
There was a problem hiding this comment.
"Get reads and streams ..."
system/system.proto
Outdated
| @@ -305,7 +285,7 @@ message Package { | |||
| // For system packages this will take effect after reboot. | |||
| bool activate = 5; | |||
There was a problem hiding this comment.
I still wonder if activate is the right name for this field -- to me a true value here could be interpreted as "make this package active now". does something like requires_reboot make sense ?
There was a problem hiding this comment.
I am open to a new name, just not sure what that might be :)
For more context, when you transfer a file, the file can be stored on the device, but not applied. So after transferring a file (without activation) a reboot of the device would not set the package to active. You have to specify somehow to set a specific package active.
There was a problem hiding this comment.
I think I'm getting hung up on the "reboot" part of this -- maybe we can change the comment to say something like:
Indicates that the package should be made active after receipt on the device.
For system image packages, the new image is expected to be active after a reboot.
|
|
||
| // RemoteDownload defines the details for a device to initiate a file transfer | ||
| // from or to a remote location. | ||
| message RemoteDownload { |
There was a problem hiding this comment.
Sorry if I missed this, but why do you think this message is common to all the services? What is the reason behind creating this file? Sorry this is just me understanding...
There was a problem hiding this comment.
No problem at all. It was originally in system.proto. In this change we wanted to add it to file.proto as well so it made sense to put it in a common place.
file/file.proto
Outdated
| // An error is returned if the file does not exist, the file transfer fails, | ||
| // or if there was an error reading the file. This is a blocking call until | ||
| // the file transfer is complete. | ||
| rpc GetRemote(GetRemoteRequest) returns (GetRemoteResponse) {} |
There was a problem hiding this comment.
Wouldnt "TransferToRemote" or something similar a better name? Sorry not trying to be obsessed with the name, but having "Get" in the name of the RPC is a bit confusing.
There was a problem hiding this comment.
I'm pretty open to a different name. Maybe TransferFromRemote would be more obvious here?
@aashaikh @robshakir Any thoughts on the name here as well? I know we had discussed this previously as well.
There was a problem hiding this comment.
I'm fine with TransferFromRemote or even FetchRemote here, but I don't have strong feelings.
The "get" in GetRemote is essentially "get this file from a remote place" - maybe fetch is better than "get" here?
There was a problem hiding this comment.
I'm leaning towards TransferFromRemote, but please let me know if you feel otherwise.
There was a problem hiding this comment.
After thinking again about it, I agree with @aghaffarkhah on TransferToRemote :)
I have updated using this name now. I think this is much more obvious than GetRemote.
file/file.proto
Outdated
| // An error is returned if the file does not exist, the file transfer fails, | ||
| // or if there was an error reading the file. This is a blocking call until | ||
| // the file transfer is complete. | ||
| rpc GetRemote(GetRemoteRequest) returns (GetRemoteResponse) {} |
There was a problem hiding this comment.
I'm fine with TransferFromRemote or even FetchRemote here, but I don't have strong feelings.
The "get" in GetRemote is essentially "get this file from a remote place" - maybe fetch is better than "get" here?
|
@robshakir @aghaffarkhah Can you please take another look? Thanks, |
@aashaikh @robshakir Can you please review this?