Skip to content

Add skip_destory flag for Fleet Agent Policies #357

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

Merged
merged 4 commits into from
Jun 14, 2023

Conversation

taylor-swanson
Copy link
Contributor

Add a flag which can skip the attempt to remove the Agent Policy from Fleet using the API. This is useful in cases where an Agent is not managed directly by Terraform and it is not safe to remove the policy, as there may still be agents assigned to the policy.

- Add a flag which can skip the attempt to remove the Agent Policy
from Fleet using the API. This is useful in cases where an Agent
is not managed directly by Terraform and it is not safe to remove
the policy, as there may still be agents assigned to the policy.
@taylor-swanson taylor-swanson self-assigned this Jun 9, 2023
@taylor-swanson taylor-swanson added the enhancement New feature or request label Jun 9, 2023
@taylor-swanson taylor-swanson marked this pull request as ready for review June 9, 2023 14:03
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

Could you please add an acceptance test for this. I think it would be good to have an assertion that the state associated with the resource is gone. (Not knowing the TF SDK well, I wasn't sure if a d.SetID("") is required, and a test will confirm this.)

@taylor-swanson
Copy link
Contributor Author

taylor-swanson commented Jun 9, 2023

Could you please add an acceptance test for this. I think it would be good to have an assertion that the state associated with the resource is gone. (Not knowing the TF SDK well, I wasn't sure if a d.SetID("") is required, and a test will confirm this.)

I did test it manually and it worked as expected. Only reason why I was opposed to adding an acceptance test for it was I didn't care for the idea of leaving around resources after a test. As long as we are okay with that, it's trivial to add the acceptance test and assert the resource is still there.

Any thoughts on this, @tobio? Never mind, we figured out how to clean up left over resources after this test.

I wasn't sure if a d.SetID("") is required

I'm a little fuzzy on this as well. I think it's redundant here since we are already actioning on deleting the resource from state. What we're bypassing is the call out to fleet to delete.

@andrewkroh
Copy link
Member

As long as we are okay with that, it's trivial to add the acceptance test and assert the resource is still there.

I would rather have it cleaned up. I think the test could do this though. Use one of the hooks, verify through the API that the resource does exist, and then delete it.

Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

LGTM. Not totally on board with the debug log level, I'd prefer it to be info, wdyt?

),
},
},
})
}

func testAccResourceAgentPolicyCreate(id string) string {
func TestAccResourceAgentPolicySkipDestroy(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@@ -264,6 +270,11 @@ func resourceAgentPolicyRead(ctx context.Context, d *schema.ResourceData, meta i
}

func resourceAgentPolicyDelete(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
if d.Get("skip_destroy").(bool) {
tflog.Debug(ctx, "Skipping destroy of Agent Policy", map[string]interface{}{"policy_id": d.Id()})
Copy link
Member

Choose a reason for hiding this comment

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

IMO this should be at a higher log level so it's clear to users that we're not destroying this policy during apply.

@taylor-swanson
Copy link
Contributor Author

LGTM. Not totally on board with the debug log level, I'd prefer it to be info, wdyt?

I made it debug level based on what I saw in the AWS provider (here's an example: https://github.com./hashicorp/terraform-provider-aws/blob/70260ef76efac73597e49841908e23e574d478e0/internal/service/logs/group.go#L211)

Since it's an opt-in option, it'll never be enabled by default, so I'm okay with it being at debug level for this reason. Also, logs won't appear unless a user opts in with TF_LOG=level.

Once this PR is merged, could we get a release cut for us (a 0.6.2, for instance)? This is currently blocking progress on us adopting the provider.

@tobio tobio merged commit fb91151 into elastic:main Jun 14, 2023
@tobio
Copy link
Member

tobio commented Jun 14, 2023

could we get a release cut for us (a 0.6.2, for instance)?

I'll look at cutting a release this week.

@tobio
Copy link
Member

tobio commented Jun 19, 2023

@taylor-swanson @ebeahan 0.6.2 got pushed out today.

@taylor-swanson taylor-swanson deleted the fleet-agent-policy-skip-destroy branch June 20, 2023 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants