Skip to content
New issue

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

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

Already on GitHub? # to your account

Update ha_datastore_apd_response_delay defaults and docs #1542

Merged
merged 3 commits into from
Jan 6, 2022
Merged

Update ha_datastore_apd_response_delay defaults and docs #1542

merged 3 commits into from
Jan 6, 2022

Conversation

tenthirtyam
Copy link
Collaborator

@tenthirtyam tenthirtyam commented Dec 20, 2021

Description

  • Updates ha_datastore_apd_response_delay to the API default for vmTerminateDelayForAPDSec.
  • Updates the compute_cluster resource documentation to seconds versus minutes for ha_datastore_apd_response_delay.
  • Updates the compute_cluster resource documentation to use the same structure for describing time-based arguments .

Release Note

Release note for CHANGELOG:

Updates `ha_datastore_apd_response_delay` in the `compute_cluster` resource to the API default (180) for `vmTerminateDelayForAPDSec`.

References

- Updates `ha_datastore_apd_response_delay` to the API default for `vmTerminateDelayForAPDSec`.
- Updates the `compute_cluster` resource documentation to seconds versus minutes for `ha_datastore_apd_response_delay`.
- Updates the `compute_cluster` resource documentation to use the same structure for describing time-based arguments .

Reference:
- Issue:  #1438
- API: https://developer.vmware.com/apis/361/doc/vim.cluster.VmComponentProtectionSettings.html#vmTerminateDelayForAPDSec

Signed-off-by: Ryan Johnson <johnsonryan@vmware.com>
@github-actions github-actions bot added documentation Type: Documentation provider Type: Provider size/s Relative Sizing: Small labels Dec 20, 2021
Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

Normally changing defaults typically could be interpreted as a breaking change if it significantly changes the behavior of the system, but considering we are adjusting a delay, and it appears the old value was erroneously calculated in minutes when the code uses it as seconds, this change is fine, just update the description on in the Go code.

Maybe one day we will generate the docs from the descriptions so they don't need to be double maintained... sigh

vsphere/resource_vsphere_compute_cluster.go Outdated Show resolved Hide resolved
@appilon
Copy link
Contributor

appilon commented Jan 5, 2022

Actually... might have to do a state migration to make this non-breaking... ugh. I need a refresher on the nuance of state here, let me created an isolated example and get back to you.

Co-authored-by: appilon <apilon@hashicorp.com>
@tenthirtyam
Copy link
Collaborator Author

Thanks for taking a look into this one, I've committed your changes in the review.

I didn't think about the possible impact to state changes.😬

Ryan

@appilon
Copy link
Contributor

appilon commented Jan 5, 2022

In practical terms this should be an easy change, but the fact that all attributes are committed to state it could cause a bad user experience/diff for the users, might be able to migrate their state from 3 to 180 if their config was left empty (implying they were relying on the default). The problem is the old SDK (which this provider uses) doesn't do a good job of providing config vs state...

@tenthirtyam
Copy link
Collaborator Author

I suppose the changes in GH-1460 for the TF SDK don't help with that concern?

@appilon
Copy link
Contributor

appilon commented Jan 5, 2022

Unfortunately no the new SDK I refer to is this one https://github.com/hashicorp/terraform-plugin-framework. And it won't be feasible to migrate to it for this provider, not to get sidetracked. Lemme see what I can repro in an isolated example.

@appilon
Copy link
Contributor

appilon commented Jan 6, 2022

Okay so I ran a few scenarios in an isolated environment.

  1. In the case where state is 180 but the default of 3 causes a diff (as described in ha_datastore_apd_response_delay units issue #1438) the change in this PR fixes the situation completely. It's odd though, I traced the code and I would NOT expect this to be the scenario must users are in, a default of 3 should have been sent to the API, Create/Update call Read to sync state, where I would expect 3 to be read back (not 180).

  2. This is the case that I expect to see, where 3 is currently in state, if we switch the default to 180, then the user will be prompted with a state drift.

Note: Objects have changed outside of Terraform

Terraform detected the following changes made outside of Terraform since the last "terraform apply":

  # bug_example.test has changed
  ~ resource "bug_example" "test" {
        id    = "example"
      ~ thing = 3 -> 180
    }

If the user applies, then there are no subsequent messages/problems.

Given that, I think it's safe to merge this PR without any state migrations, we will make note in the changelog that a onetime drift message is possible. I did notice there is another piece of schema that I guess mirrors this one but for overrides?

@tenthirtyam Can you perform the doc updates here and here as well?

@tenthirtyam
Copy link
Collaborator Author

@appilon I'll make these needed changes and update the PR very soon. Thanks!

Ryan

- Updates the documentation for the `ha_datastore_apd_response_delay` time interval from minutes to seconds.
- Update resource documentation reference `ref-vsphere-ha-clusters` to vSphere 7.0.

Signed-off-by: Ryan Johnson <johnsonryan@vmware.com>
@tenthirtyam
Copy link
Collaborator Author

Committed the changes for the ha_datastore_apd_response_delay to denote the time interval from minutes to seconds.

@appilon appilon merged commit f7af496 into hashicorp:master Jan 6, 2022
@tenthirtyam
Copy link
Collaborator Author

Thanks for the support @appilon!

@tenthirtyam tenthirtyam deleted the gh-1438 branch January 6, 2022 16:14
@github-actions
Copy link

github-actions bot commented Feb 6, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 6, 2022
@tenthirtyam tenthirtyam added this to the v2.1.0 milestone Feb 14, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
documentation Type: Documentation provider Type: Provider size/s Relative Sizing: Small
Projects
None yet
2 participants