-
Notifications
You must be signed in to change notification settings - Fork 25
r/opc_storage_object: Add opc_storage_object
resource
#55
Conversation
Still need to add timeouts to the resource, but those can be added later. Also need to add Import and Import test ``` $ make testacc TEST=./opc TESTARGS="-run=TestAccOPCStorageObject" ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./opc -v -run=TestAccOPCStorageObject -timeout 120m === RUN TestAccOPCStorageObject_contentSource --- PASS: TestAccOPCStorageObject_contentSource (17.08s) === RUN TestAccOPCStorageObject_fileSource --- PASS: TestAccOPCStorageObject_fileSource (12.49s) PASS ok github.com/terraform-providers/terraform-provider-opc/opc 29.577s ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments - otherwise LGTM :)
opc/resource_storage_object.go
Outdated
Type: schema.TypeString, | ||
Optional: true, | ||
ForceNew: true, | ||
Description: "Set the MIME type for the object", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a default here - or does the API handle that for us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API handles this for us if left unspecified
Optional: true, | ||
Computed: true, | ||
ForceNew: true, | ||
Description: "Specify the number of seconds after which the system deletes the object", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
number of seconds
I don't know the use-case for this, but would it make sense to put this in a larger unit? We also have this in the disk sizes, which are submitted in Bytes but are either MB/GB in the Schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly... I'd almost opt for adding an interpolation function to core
that converts hours to minutes, and one that converts minutes to seconds, so a user can fully specify seconds if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of an interpolation function for this
opc/resource_storage_object.go
Outdated
input.CopyFrom = v.(string) | ||
} else { | ||
// One of the three attributes are required | ||
return fmt.Errorf("Must specify %q, %q, or %q field", "file", "copy_from", "content") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do these field names want ` quotes around them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, can use those instead
opc/resource_storage_object.go
Outdated
func resourceOPCStorageObjectDelete(d *schema.ResourceData, meta interface{}) error { | ||
client := meta.(*OPCClient).storageClient.Objects() | ||
if client == nil { | ||
return fmt.Errorf("Storage client is not initialized. Make sure to use `storage_endpoint` variable or the `OPC_STORAGE_ENDPOINT` environment variable") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: is it worth making this error message a constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely 😄
|
||
const _TestStorageObjectPath = "test-fixtures" | ||
|
||
func TestAccOPCStorageObject_contentSource(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given fields like content_type
are Optional but always supplied in the test - it'd be good to add a "basic" test with only the required fields set if possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aye, agreed. Will add
- Adds constant to capture nil client errors - Adds new test for a computed `content_type` attribute - Allows `content_type` to be `Computed` ``` $ make testacc test=./opc TESTARGS="-run=TestAccOPCStorageObject_contentSource_nilContentType" ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccOPCStorageObject_contentSource_nilContentType -timeout 120m ? github.com/terraform-providers/terraform-provider-opc [no test files] === RUN TestAccOPCStorageObject_contentSource_nilContentType --- PASS: TestAccOPCStorageObject_contentSource_nilContentType (15.58s) PASS ok github.com/terraform-providers/terraform-provider-opc/opc 15.582s ```
Adds import functionality and an import test. Should note in documentation, that neither `content`, `file`, nor `copy_from` are ever returned from the Oracle API, so importing a storage object will only allow a user to delete the object, or create a new object with different content. ``` $ make testacc test=./opc TESTARGS="-run=TestAccOPCStorageObject_importBasic" ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccOPCStorageObject_importBasic -timeout 120m ? github.com/terraform-providers/terraform-provider-opc [no test files] === RUN TestAccOPCStorageObject_importBasic --- PASS: TestAccOPCStorageObject_importBasic (16.10s) PASS ok github.com/terraform-providers/terraform-provider-opc/opc 16.103s ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
Still need to add timeouts to the resource, but those can be added later.
Also need to add Import and Import test