feat(audit-trail): add support for audit trail to terraform provider #495

mwasilew2 · 2024-01-08T17:51:25Z

Description of the change

closes: https://app.clickup.com/t/8693fq8pf

blocked on:

~~expose id~~ won't be needed:
- ~~https://github.com/spacelift-io/backend/pull/6147~~
- ~~https://github.com/spacelift-io/backend/pull/6165~~
https://github.com/spacelift-io/backend/pull/6154

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation (non-breaking change that adds documentation)

Related issues

Fix #1

Checklists

Development

Lint rules pass locally
The code changed/added as part of this pull request has been covered with tests
All tests related to the changed code pass in development
Examples for new resources and data sources have been added
Default values have been documented in the description (e.g., "Dummy: (Boolean) Blah blah. Defaults to false.)
If the action fails that checks the documentation: Run go generate to make sure the docs are up to date

Code review

This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
Pull Request is no longer marked as "draft"
Reviewers have been assigned
Changes have been reviewed by at least one other engineer

Clickup-user · 2024-01-08T17:51:29Z

Task linked: CU-8693fq8pf add support for auditTrailWebhook to terraform provider

jakubdal

It looks like a neat copy-paste with intention behind it. However I don't know how a provider should be implemented properly, so an encouraging comment is best I can do 💪

mwasilew2 · 2024-01-22T10:25:41Z

@peterdeme @michalg9 I saw you working on terraform provider recently, you would mind taking a look?

peterdeme · 2024-01-22T10:26:02Z

spacelift/resource_audit_trail_webhook.go

+		return diag.Errorf("could not create audit trail webhook: %v", internal.FromSpaceliftError(err))
+	}
+
+	data.SetId(time.Now().String())


Are you sure about this one?

@peterdeme

nope, not at all, I mean it works, but it's a hack, see: https://spacelift-io.slack.com/archives/CRTS17WLF/p1705055720419239

I also drafted PRs to actually execute the "ulid based" solution: https://github.com/spacelift-io/backend/pull/6165

I see. I don't have a strong opinion on this, so feel free to do whatever :D Personally, I would either generate a "smart" id (maybe from the endpoint URL?), or go through with the ULID pull request.

Waiting on other people's opinion.

Personally if it works I'm fine with this approach. There's only ever one audit trail webhook, and I don't think there's a reason to need to import the resource (since the mutation works like an upsert).

mwasilew2 · 2024-01-23T08:50:36Z

@marcinwyszynski @adamconnelly I saw you've been active in the repo, could you take a look? Thanks in advance

adamconnelly

Looks good overall. Left some comments, but nothing major.

adamconnelly · 2024-01-23T15:03:44Z

spacelift/resource_audit_trail_webhook.go

+func resourceAuditTrailWebhook() *schema.Resource {
+	return &schema.Resource{
+		Description: "" +
+			"`spacelift_audit_trail_webhook` represents a webhook endpoint to which Spacelift " +


Couple of suggestions here:

Maybe we should say "sends POST requests" instead of "sends the POST request"?

I'm not sure about the "events the user wants to track" part. It kinda implies that you can choose the events you're interested in. Maybe we should just say "about audit events" or something like that?

adamconnelly · 2024-01-23T15:06:03Z

spacelift/resource_audit_trail_webhook.go

+			StateContext: schema.ImportStatePassthroughContext,
+		},
+
+		Schema: map[string]*schema.Schema{


Can you add descriptions to these fields please?

adamconnelly · 2024-01-23T15:58:06Z

spacelift/resource_audit_trail_webhook.go

+		return diag.Errorf("could not create audit trail webhook: %v", internal.FromSpaceliftError(err))
+	}
+
+	data.SetId(time.Now().String())


Personally if it works I'm fine with this approach. There's only ever one audit trail webhook, and I don't think there's a reason to need to import the resource (since the mutation works like an upsert).

adamconnelly · 2024-01-23T16:01:04Z

spacelift/resource_audit_trail_webhook_test.go

+}
+`
+
+func Test_resourceAuditTrailWebhook(t *testing.T) {


Might wanna test deletion as well.

Deletion should be a part of each test (the test will fail if the resource created for the test was not successfully removed). Do you reckon we need to test it explicitly? Do you know how to test it explicitly? I don't see a test like that in the repo and I'm not sure how to do it.

That's fine then - don't worry about it if the test fails anyway if deletion fails. I just hadn't realised that.

As for how you'd do it - I imagine you'd just add a test step that applied a configuration where the resource wasn't included.

As for how you'd do it - I imagine you'd just add a test step that applied a configuration where the resource wasn't included.

yeah, just wasn't sure how to test for a missing resource after applying an empty config

adamconnelly · 2024-01-23T16:01:51Z

spacelift/resource_audit_trail_webhook_test.go

+		})
+	})
+
+	t.Run("cannot change endpoint", func(t *testing.T) {


Is this not more a test of the API rejecting an endpoint that doesn't exist, rather than testing that the endpoint can't be changed (which should totally be possible)?

testing that the endpoint can't be changed (which should totally be possible)?

It's not possible to edit the endpoint if the audit trail webhook is enabled.

You sure? Looking at the backend code, it seems to allow that, and just sends to both old and new endpoints if that's the case.

Regardless, I don't think this test is verifying that the endpoint can't be changed. It's just checking that if you set the endpoint to one that doesn't exist, we don't allow the endpoint to be updated.

you're right, I think the reason I was able to set the endpoint to a non-existent value was that I disabled audit trail first and the check is only performed if it's enabled.

thanks!

I think the test still brings some value so I'll leave it and update the name.

Signed-off-by: Michal Wasilewski <[email protected]>

…495) Signed-off-by: Michal Wasilewski <[email protected]>

mwasilew2 marked this pull request as ready for review January 15, 2024 13:42

mwasilew2 requested review from michieldewilde, KNiepok and jakubdal January 15, 2024 13:42

mwasilew2 force-pushed the CU-8693fq8pf_add-support-for-auditTrailWebhook-to-terraform-provider_Michal-Wasilewski branch from 69094c2 to edeeded Compare January 15, 2024 13:47

jakubdal reviewed Jan 22, 2024

View reviewed changes

mwasilew2 requested review from michalg9 and peterdeme January 22, 2024 10:24

peterdeme reviewed Jan 22, 2024

View reviewed changes

mwasilew2 requested review from marcinwyszynski and adamconnelly January 23, 2024 08:49

mwasilew2 mentioned this pull request Jan 23, 2024

feat(security-email): add support for security email #497

Merged

14 tasks

adamconnelly reviewed Jan 23, 2024

View reviewed changes

mwasilew2 requested a review from adamconnelly January 25, 2024 10:45

adamconnelly approved these changes Jan 25, 2024

View reviewed changes

mwasilew2 force-pushed the CU-8693fq8pf_add-support-for-auditTrailWebhook-to-terraform-provider_Michal-Wasilewski branch from 43b74c6 to 64582ae Compare January 25, 2024 12:22

feat(audit-trail): add support for audit trail to terraform provider

e2d18b0

Signed-off-by: Michal Wasilewski <[email protected]>

mwasilew2 force-pushed the CU-8693fq8pf_add-support-for-auditTrailWebhook-to-terraform-provider_Michal-Wasilewski branch from 64582ae to e2d18b0 Compare January 25, 2024 12:51

mwasilew2 merged commit 921dad3 into main Jan 25, 2024
7 checks passed

mwasilew2 deleted the CU-8693fq8pf_add-support-for-auditTrailWebhook-to-terraform-provider_Michal-Wasilewski branch January 25, 2024 13:05

truszkowski pushed a commit that referenced this pull request Mar 4, 2024

feat(audit-trail): add support for audit trail to terraform provider (#…

9070945

…495) Signed-off-by: Michal Wasilewski <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(audit-trail): add support for audit trail to terraform provider #495

feat(audit-trail): add support for audit trail to terraform provider #495

mwasilew2 commented Jan 8, 2024 •

edited

Loading

Clickup-user commented Jan 8, 2024

jakubdal left a comment

mwasilew2 commented Jan 22, 2024

peterdeme Jan 22, 2024

mwasilew2 Jan 22, 2024

mwasilew2 Jan 22, 2024

peterdeme Jan 22, 2024 •

edited

Loading

adamconnelly Jan 23, 2024

mwasilew2 commented Jan 23, 2024

adamconnelly left a comment

adamconnelly Jan 23, 2024

adamconnelly Jan 23, 2024

adamconnelly Jan 23, 2024

adamconnelly Jan 23, 2024

mwasilew2 Jan 25, 2024

adamconnelly Jan 25, 2024

mwasilew2 Jan 25, 2024

adamconnelly Jan 23, 2024

mwasilew2 Jan 25, 2024

adamconnelly Jan 25, 2024

mwasilew2 Jan 25, 2024 •

edited

Loading

feat(audit-trail): add support for audit trail to terraform provider #495

feat(audit-trail): add support for audit trail to terraform provider #495

Conversation

mwasilew2 commented Jan 8, 2024 • edited Loading

Description of the change

Type of change

Related issues

Checklists

Development

Code review

Clickup-user commented Jan 8, 2024

jakubdal left a comment

Choose a reason for hiding this comment

mwasilew2 commented Jan 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterdeme Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwasilew2 commented Jan 23, 2024

adamconnelly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwasilew2 Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

mwasilew2 commented Jan 8, 2024 •

edited

Loading

peterdeme Jan 22, 2024 •

edited

Loading

mwasilew2 Jan 25, 2024 •

edited

Loading