Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added expiration date field to ibm_kms_key resource #1967

Merged
merged 6 commits into from
Oct 21, 2020
Merged

Added expiration date field to ibm_kms_key resource #1967

merged 6 commits into from
Oct 21, 2020

Conversation

Sidd-kp
Copy link
Contributor

@Sidd-kp Sidd-kp commented Oct 15, 2020

Description : Added expiration date functionality to key creation in ibm_kms_key and made appropriate changes in test file for testing.

@Sidd-kp Sidd-kp changed the title Kp dte Added expiration date field to ibm_kms_key resource Oct 15, 2020
Copy link
Collaborator

@dinesh-venkatraman dinesh-venkatraman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since expiration_date is a date field, the Default value has to be a default date or as a null. Setting a string value is not necessary. You can check if any default value is set in KMK or set this a null

@Sidd-kp
Copy link
Contributor Author

Sidd-kp commented Oct 15, 2020

So the default value is null but we cannot assign a string as null. The word Default is just a placeholder. Later in the code I have handled it and am sending a null value if 'Default' is not set to any date.

Copy link
Collaborator

@hkantare hkantare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the argument in doc
website/docs/r/kms_key.html.markdown

Optional: true,
Description: "The date the key material expires. The date format follows RFC 3339. You can set an expiration date on any key on its creation. A key moves into the Deactivated state within one hour past its expiration date, if one is assigned. If you create a key without specifying an expiration date, the key does not expire",
ForceNew: true,
Default: "Default",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't set any Default value....since the API doesn't have any Default value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Will do so.

@@ -185,13 +192,23 @@ func resourceIBMKmsKeyCreate(d *schema.ResourceData, meta interface{}) error {
kpAPI.Config.InstanceID = instanceID
name := d.Get("key_name").(string)
standardKey := d.Get("standard_key").(bool)

expiration_string := d.Get("expiration_date").(string)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For optional fields use d.GetOk()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

expiration_string := d.Get("expiration_date").(string)
var expiration_time time.Time
expiration := &expiration_time
if expiration_string != "Default" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So all these logic can be moved loop d.GetOk("XXXXX");ok {
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure that would be cleaner.

@@ -125,7 +137,7 @@ func testAccCheckIBMKmsResourceRootkeyWithCOSConfig(instanceName, KeyName, cosIn
key_protect = ibm_kms_key.test.id
}

`, instanceName, KeyName, cosInstanceName, bucketName)
`, instanceName, KeyName, expirationdate, cosInstanceName, bucketName)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't change all existing testcase because we are missing the flow of expiration nil

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which ones should I revert?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or would I need to write my own test cases?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let the old testcase remain same..write a new testcase

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@Sidd-kp
Copy link
Contributor Author

Sidd-kp commented Oct 20, 2020

Screenshot 2020-10-20 at 3 26 42 PM

Screenshot 2020-10-20 at 3 27 08 PM

@hkantare hkantare merged commit cadfd6b into IBM-Cloud:master Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants