[Resource] Deprecate Snapshot For Instance Snapshot #5675
Conversation
ismirlia
requested review from
yussufsh,
hkantare,
michaelkad and
Alexander-Kita
|
ismirlia
force-pushed
the
snapshots-deprecation
branch
from
79886b1 to
68a3f90
Compare
ismirlia
force-pushed
the
snapshots-deprecation
branch
from
52a8928 to
d3541d7
Compare
|
@yussufsh Was there any other comment on this? Is this mergeable? (I squashed the commits recently) |
ismirlia
force-pushed
the
snapshots-deprecation
branch
from
5268091 to
ab16f36
Compare
|
@yussufsh can you merge this? |
…m into snapshots-deprecation
…m into snapshots-deprecation
|
@yussufsh can you approve this PR |
| client := instance.NewIBMPISnapshotClient(ctx, sess, cloudInstanceID) | ||
| snapshot, err := client.Get(snapshotID) | ||
| if err != nil { | ||
| // snapshot does not exist |
There was a problem hiding this comment.
Did you mean to add a TODO comment?
Check if the error code is 404.
There was a problem hiding this comment.
I'm not following the issue in this line. Do you want me to check the error code of the get and only set the id to empty if the we get a 404 vs another unrelated reason the get failed? This step of getting the snapshot seems strange already because I'm not seeing other resources get the item they want to delete before calling delete. Before this PR, the last edit to this was here: https://github.com/IBM-Cloud/terraform-provider-ibm/pull/3429/files#diff-317acb20df0113077a2bc534ea43860b874afacdb0685550b7a64907721bec76 where it was assumed previously if we couldn't get it we should error out because it should still exist at that point to terraform before we delete it.
I don't think I mentioned this earlier. The main goal of this PR was to create a new resource called ibm_pi_instance_snapshot to replace the existing ibm_pi_snapshot because it called the instance snapshot api and not the snapshot api. I didn't touch any of the code or documentation in the snapshot ibm_pi_snapshot resource unless it was to add something new we needed (like user tags). I'm more than happy to make any tweaks to how this works, but I did not write this originally so I don't know why it was written this way. 🙂
There was a problem hiding this comment.
If I recall it correctly the delete api did not have a 404 return code hence we were relying on get api before deleting. If delete can return 404 we can remove the get call.
I'm not sure how that comment ended up there but this needs cleaning. That is why mentioned here.
There was a problem hiding this comment.
Ah I see, I'll double check it.
/pcloud/v1/cloud-instances/{cloud_instance_id}/snapshots/{snapshot_id}:
delete:
operationId: pcloud.cloudinstances.snapshots.delete
responses:
"202":
description: Accepted
schema:
$ref: '#/definitions/Object'
"400":
description: Bad Request
schema:
$ref: '#/definitions/Error'
"401":
description: Unauthorized
schema:
$ref: '#/definitions/Error'
"403":
description: Forbidden
schema:
$ref: '#/definitions/Error'
"404":
description: Not Found
schema:
$ref: '#/definitions/Error'
"410":
description: Gone
schema:
$ref: '#/definitions/Error'
"500":
description: Internal Server Error
schema:
$ref: '#/definitions/Error'
It looks like we have a 404 now, so I'll remove the check.
| - `pi_instance_name` - (Required, String) The name of the instance you want to take a snapshot of. | ||
| - `pi_snapshot_name` - (Required, String) The unique name of the snapshot. | ||
| - `pi_user_tags` - (Optional, List) The user tags attached to this resource. | ||
| - `pi_volume_ids` - (Optional, String) A list of volume IDs of the instance that will be part of the snapshot. If none are provided, then all the volumes of the instance will be part of the snapshot. |
There was a problem hiding this comment.
Should not this be List of strings?
Also saw another instance below
`volume_snapshots` - (String) A map of volume snapshots included in the PVM instance snapshot.
How can a map be string type?
I am going to merge this PR, please correct in future PR.
There was a problem hiding this comment.
Output from acceptance testing: