-
Notifications
You must be signed in to change notification settings - Fork 396
Add App Key Registration Resource #3054
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
base: master
Are you sure you want to change the base?
Conversation
go 1.23.0 | ||
|
||
replace github.com/DataDog/datadog-api-client-go/v2 => ../datadog-api-client-go |
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.
Remove once public API PR gets merged.
_ resource.ResourceWithImportState = &appKeyRegistrationResource{} | ||
) | ||
|
||
type appKeyRegistrationResource struct { |
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.
should we have a data source for this too? up to you if it makes sense or not.
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.
It doesn't make sense there is nothing to read the only property is the ID
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.
It might be useful if someone wants to reference an App Key that isn't TF-managed from another TF resource. If that's not useful then yea a data source wouldn't make sense.
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.
My thought was that this would be done by the app key data source then, not a app key registration data source.
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.
can you add tests for this?
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.
Yea
Attributes: map[string]schema.Attribute{ | ||
"id": schema.StringAttribute{ | ||
Description: "The Application Key ID to register.", | ||
Required: true, |
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.
shouldn't this be set as Computed
instead? unless the user is expected to provide their own UUID.
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.
This should not be computed. We need them to enter the ID it is the only property.
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.
kk that's fine we just usually generate IDs on the server that's why I asked
return | ||
} | ||
|
||
// If the app key registration is not found, we log a warning and remove the resource from state. This may be due to changes in the UI or attempting to import an app key that is not registered. |
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 think having a "read" remove the resource from state is unintuitive. I think we should keep it as a 404 and it'll be on the customer to remove any non-existent registrations from their TF config.
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.
No I don't think so. This actually standard I believe. The reason is if the customer deletes it in the UI and you run any terraform command (like plan) your commands will fail and it is unrecoverable unless they reregister in the UI. The plan will display the user that the terraform resource will be deleted on the next apply.
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.
sounds good to me
@embeaken thanks for taking a look. |
@@ -31,7 +31,7 @@ func (d *actionConnectionDatasource) Metadata(_ context.Context, request datasou | |||
|
|||
func (d *actionConnectionDatasource) Schema(_ context.Context, request datasource.SchemaRequest, response *datasource.SchemaResponse) { | |||
response.Schema = schema.Schema{ | |||
Description: "A connection that can be used in Actions, including in the Workflow Automation and App Builder products.", | |||
Description: "A connection that can be used in Actions, including in the Workflow Automation and App Builder products. This data source requires a [registered application key](https://registry.terraform.io/providers/DataDog/datadog/latest/docs/resources/app_key_registration).", |
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.
This link gives me a Page Not Found error on the Terraform site. I assume the link is not ready yet?
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.
Yes the link will only work once this is merged.
Ticket: 10685
Motivation: https://docs.google.com/document/d/1GxqhcJLP2RTxqZZBed5lLp2mEehDA1K_Lu_FNgKH0qM/edit?tab=t.0#heading=h.g96a7txsgj2p
Dependent on public API release. This PR adds the new app key registration resource.