bugfix: change volume metadata struct #1271
Conversation
storage/volume/types/volume.go
Outdated
| @@ -83,10 +83,12 @@ type VolumeConfig struct { | |||
|
|
|||
| // VolumeSpec represents volume spec. | |||
| type VolumeSpec struct { | |||
| ClusterAPI string `json:"clusterapi"` | |||
There was a problem hiding this comment.
Quite confused on the naming of this field. ClusterAPI is some kind of network address?
If that, how about adding ClusterAPIAddress ?
| @@ -6,10 +6,10 @@ import ( | |||
|
|
|||
| // Config represents volume config struct. | |||
| type Config struct { | |||
| ControlAddress string | |||
There was a problem hiding this comment.
ControlServerAddress or ControlMasterAddress or MasterAddress?
There was a problem hiding this comment.
it means control server address in csi. I change it to ControlServerAddress
| @@ -6,10 +6,10 @@ import ( | |||
|
|
|||
| // Config represents volume config struct. | |||
| type Config struct { | |||
| ControlAddress string | |||
| Timeout time.Duration // operation timeout. | |||
| RemoveVolume bool | |||
There was a problem hiding this comment.
I am wondering if we could add some annotations for this field RemoveVolume ?
I wish that when code readers reader the struct itself, he could catch the exact meaning of it clearly, rather than trying hard to guess its meaning.
If we make it, I think the lifetime of the code would be much longer to be alive.
rudyfly
force-pushed
the
volume-remote
branch
2 times, most recently
from
31b43c7
to
696adbd
Compare
Codecov Report
@@ Coverage Diff @@
## master #1271 +/- ##
=========================================
- Coverage 15.4% 15.21% -0.2%
=========================================
Files 174 172 -2
Lines 11026 10734 -292
=========================================
- Hits 1699 1633 -66
+ Misses 9208 8980 -228
- Partials 119 121 +2
|
storage/volume/types/storage.go
Outdated
| PoolSpec map[string]PoolSpec `json:"poolspec"` // storage pool spec | ||
| Type string `json:"type"` // storage type | ||
| ID string `json:"id,omitempty"` // storage uid or unique name | ||
| API string `json:"api"` // gateway address |
There was a problem hiding this comment.
If it means gateway address, could we just use GatewayAddress?
I felt that API is so ambiguous. 😢
There was a problem hiding this comment.
If this filed is only used in CSI master, and will not be used in pouch, then maybe we need to remove this. WDYT? @rudyfly
change volume metadata struct for remote storage manager. Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>
|
LGTM |
pouchrobot
added
the
LGTM
Ⅰ. Describe what this PR did
change volume metadata struct for remote storage manager.
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews
Signed-off-by: Rudy Zhang rudyflyzhang@gmail.com