Skip to content

first steps for kms key-ring resource and datasource #897

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

ruslan-18
Copy link

Description

relates to #1234

Checklist

  • Issue was linked above
  • Code format was applied: make fmt
  • Examples were added / adjusted (see examples/ directory)
  • Docs are up-to-date: make generate-docs (will be checked by CI)
  • Unit tests got implemented or updated
  • Acceptance tests got implemented or updated (see e.g. here)
  • Unit tests are passing: make test (will be checked by CI)
  • No linter issues: make lint (will be checked by CI)

Copy link

github-actions bot commented Jul 9, 2025

This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it.

@github-actions github-actions bot added the Stale PR is marked as stale due to inactivity. label Jul 9, 2025
@rubenhoenle rubenhoenle removed the Stale PR is marked as stale due to inactivity. label Jul 10, 2025
panic("implement me")
}

func toCreatePayload(model *Model) (*kms.CreateKeyRingPayload, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Please consider implementing a unit test for this func.

Copy link
Author

Choose a reason for hiding this comment

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

added unit test

}, nil
}

func mapFields(keyRing *kms.KeyRing, model *Model) error {
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Author

Choose a reason for hiding this comment

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

added unit test

return &keyRingResource{}
}

type keyRingResource struct {
Copy link
Member

Choose a reason for hiding this comment

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

Don't know if you are aware of this, please don't forget to implement the import.

You can ensure this by adding the code below:

// Ensure the implementation satisfies the expected interfaces.
var (
	_ resource.Resource                 = &keyRingResource{}
	_ resource.ResourceWithConfigure    = &keyRingResource{}
	_ resource.ResourceWithImportState  = &keyRingResource{}
)

See the git instance resource how to do it:

// Ensure the implementation satisfies the expected interfaces.
var (
_ resource.Resource = &gitResource{}
_ resource.ResourceWithConfigure = &gitResource{}
_ resource.ResourceWithImportState = &gitResource{}
)

Consider doing the same for the datasource, see the git instance datasource for example:

https://github.com/stackitcloud/terraform-provider-stackit/blob/main/stackit/internal/services/git/instance/datasource.go#L24-L27

Copy link
Author

Choose a reason for hiding this comment

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

done

}

func (k *keyRingResource) Metadata(ctx context.Context, request resource.MetadataRequest, response *resource.MetadataResponse) {
response.TypeName = request.ProviderTypeName + "kms_key_ring"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
response.TypeName = request.ProviderTypeName + "kms_key_ring"
response.TypeName = request.ProviderTypeName + "_kms_key_ring"

Copy link
Author

Choose a reason for hiding this comment

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

updated

KeyRingId types.String `tfsdk:"key_ring_id"`
Id types.String `tfsdk:"id"` // needed by TF
ProjectId types.String `tfsdk:"project_id"`
RegionId types.String `tfsdk:"region_id"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RegionId types.String `tfsdk:"region_id"`
Region types.String `tfsdk:"region"` # small adjustment to stick with the naming conventions across the codebase

According to https://docs.api.stackit.cloud/documentation/kms/version/v1beta , the KMS API already uses the new multi-region concept.

See e.g. the stackit_routing_table resource how to implement the multi-region concept. The resource has a region field which is marked as optional and will use the default_region configured in the provider as a fallback: https://registry.terraform.io/providers/stackitcloud/stackit/latest/docs/resources/routing_table

Copy link
Author

Choose a reason for hiding this comment

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

updated region logic

if providerData.KMSCustomEndpoint != "" {
apiClientConfigOptions = append(apiClientConfigOptions, config.WithEndpoint(providerData.KMSCustomEndpoint))
} else {
apiClientConfigOptions = append(apiClientConfigOptions, config.WithRegion(providerData.GetRegion()))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
apiClientConfigOptions = append(apiClientConfigOptions, config.WithRegion(providerData.GetRegion()))
apiClientConfigOptions = append(apiClientConfigOptions))

Since the KMS API already implemented the new multi-region concept, you don't need to set the region here (or better: the SDK should throw an error if you do 😄 ).

Apart from that, consider adding a unit tests for this func, see e.g. https://github.com/stackitcloud/terraform-provider-stackit/blob/8e776757ea2280d1222afe50c3024945b4d99eed/stackit/internal/services/git/utils/util_test.go

Copy link
Author

Choose a reason for hiding this comment

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

updated

Copy link

This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it.

@github-actions github-actions bot added the Stale PR is marked as stale due to inactivity. label Jul 24, 2025
@rubenhoenle rubenhoenle removed the Stale PR is marked as stale due to inactivity. label Jul 24, 2025
rubenhoenle added a commit to stackitcloud/stackit-sdk-go that referenced this pull request Jul 29, 2025
rubenhoenle added a commit to stackitcloud/stackit-sdk-go that referenced this pull request Aug 1, 2025
rubenhoenle added a commit to stackitcloud/stackit-sdk-go that referenced this pull request Aug 1, 2025
Copy link

github-actions bot commented Aug 5, 2025

This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it.

@github-actions github-actions bot added the Stale PR is marked as stale due to inactivity. label Aug 5, 2025
@marceljk marceljk removed the Stale PR is marked as stale due to inactivity. label Aug 5, 2025
@ruslan-18 ruslan-18 marked this pull request as ready for review August 8, 2025 13:14
@ruslan-18 ruslan-18 requested a review from a team as a code owner August 8, 2025 13:14
@ruslan-18
Copy link
Author

@fsandel fyi

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