Added changes in volume object. #228
Conversation
chitranm
requested review from
mchuang3,
dbozzato81,
karthik-radakrishnan and
neilgierman
|
LGTM. Will defer to Denis/Mike to approve. |
| @@ -231,7 +231,7 @@ require ( | |||
| golang.org/x/exp/typeparams v0.0.0-20240314144324-c7f7c6466f7f // indirect | |||
| golang.org/x/mod v0.17.0 // indirect | |||
| golang.org/x/net v0.25.0 // indirect | |||
| golang.org/x/oauth2 v0.21.0 // indirect | |||
| golang.org/x/oauth2 v0.23.0 // indirect | |||
There was a problem hiding this comment.
Why this dependency has changed?
There was a problem hiding this comment.
@dbozzato81, I didn't do anything except the change at line 9 and "go mod tidy".
There was a problem hiding this comment.
It was part of the new metal-client release:
| Required: false, | ||
| Optional: false, |
There was a problem hiding this comment.
shouldn't one of these be true?
There was a problem hiding this comment.
I will remove this Optional parameter as it is a read only field.
| Required: false, | ||
| Optional: false, |
There was a problem hiding this comment.
shouldn't one of these be true?
There was a problem hiding this comment.
There is already a vStoragePool attribute present
There was a problem hiding this comment.
Looks like vStoragePool is not being populated (in the translation layer). Wonder how it's being used by the client (BMaaS UI).
There was a problem hiding this comment.
Hi @karthik-radakrishnan and @dbozzato81, I realized that vStoragePool is getting populated using /available-resources API. And if I remove StoragePoolName then we are back to the same problem where vStoragePool can have a specific data to a particular pod location. And same is true with DataCanterName. I will keep both of them as these are read only parameters. Let me know if there are any concerns.
There was a problem hiding this comment.
From the vStoragePool definition and looking at the code, it seams it is only used for Volume create i.e. it is not getting populated by the provider by it is given by the user in case vStoragePoolID is not.
vStoragePool: {
Type: schema.TypeString,
Required: false,
Optional: true,
Description: "The storage pool of the volume to be created.",
},So why we can't use it like this?...
vStoragePool: {
Type: schema.TypeString,
Optional: true,
Computed: true,
Description: "The storage pool of the volume to be created.",
},Another question, are we sure that vStoragePoolID is computed and returned correctly for external volumes?
There was a problem hiding this comment.
Hi @dbozzato81, vStoragePoolID is assigned to externally managed volumes when we create them in the DB from DCC. This poolID is assigned based on the array volume was created on.
I have explained above for the vStoragePool - This is fetched based on the vStoragePoolID and available-resources response. And then it is used to create Volume.
vStoragePoolName and vDataCenterName are read only fields and if we don't want them to have in terraform then we can discard this PR and make vStoragePool Computed as true
| @@ -1,13 +1,14 @@ | |||
| module github.com/hewlettpackard/hpegl-metal-terraform-resources | |||
|
|
|||
| go 1.21.0 | |||
| go 1.22.0 | |||
There was a problem hiding this comment.
@neilgierman, please check if this is Ok? It is coming after "go mod tidy".
There was a problem hiding this comment.
I have another PR at #231 to be a more complete update to gp 1.22. Once that is approved, then the new master can be merged to this PR.
|
After the discussion with @dbozzato81 and @karthik-radakrishnan, we realized that these two new parameters are read only and not required to create volume resource. Hence I am discarding this PR. @neilgierman do I need to keep the go.mod and go.sum changes in this PR? |
Nothing is needed from this PR. I have #231 open that has the Go changes only. |
DE24521 - Added new read only fields in volume object - StoragePoolName and DataCenterName.