Skip to content
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

New Resource: generating Kubernetes Fleet Member #3416

Merged
merged 10 commits into from
Feb 6, 2024

Conversation

ms-henglu
Copy link
Contributor

No description provided.

@zioproto
Copy link

zioproto commented Jan 3, 2024

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks for trying to generate this resource with Pandora @ms-henglu, it is a good candidate for that.

In addition to the comments left in-line the following also needs to be done:

  1. Rebase this PR - a lot of new functionality, changes and fixes have been made since this was opened, when we try and generate this resource with the current state of the repo the generation is skipped (this was a known bug that was fixed)
  2. We need to update the common ID for fleet in go-azure-helpers to KubernetesFleetId, this is related to the comment left in-line on the fleet_id test dependency
  3. We need to add a common_id_kubernetes_fleet_id.go file to this directory and also add this new common ID to the common_ids.go file

Comment on lines 60 to 62
description = <<HERE
Manages a Kubernetes Fleet Member
HERE
Copy link
Member

Choose a reason for hiding this comment

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

Since we don't require a multi-line description in the docs, this can be simplified to

Suggested change
description = <<HERE
Manages a Kubernetes Fleet Member
HERE
description = "Manages a Kubernetes Fleet Member"

@@ -159,7 +167,9 @@ type dependencyDefinition struct {
func DetermineDependencies(field, providerPrefix string, dependencies *testDependencies) (*string, *testDependencies, error) {
dependencyMapping := map[string]dependencyDefinition{
"application_insights_id": {dependencies.setNeedsApplicationInsights, fmt.Sprintf("%s_application_insights.test.id", providerPrefix)},
"cluster_resource_id": {dependencies.setNeedsKubernetesCluster, fmt.Sprintf("%s_kubernetes_cluster.test.id", providerPrefix)},
Copy link
Member

Choose a reason for hiding this comment

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

Although this allows us to generate the required dependencies for the generated tests to work, we already have a resource ID associated with a Kubernetes Cluster dependency on line 176.

These dependencies coincide with how we refer to resource IDs in the azurerm provider. kubernetes_cluster_id follows our naming conventions and is used consistently throughout the provider, so the solution here should be to rename cluster_resource_id to kubernetes_cluster_id instead of defining duplicate dependency for the same thing.

We have the ability to do renames on properties in the resource definition now with overrides. I left a comment on resource definition config on what that should look like.

Suggested change
"cluster_resource_id": {dependencies.setNeedsKubernetesCluster, fmt.Sprintf("%s_kubernetes_cluster.test.id", providerPrefix)},

description = <<HERE
Manages a Kubernetes Fleet Member
HERE
}
Copy link
Member

@stephybun stephybun Jan 31, 2024

Choose a reason for hiding this comment

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

This will rename the property cluster_resource_id to kubernetes_cluster_id which will allow us to use the existing test dependency defined for the tests.

Suggested change
}
overrides "cluster_resource_id" {
updated_name = "kubernetes_cluster_id"
}

"dev_center_id": {dependencies.setNeedsDevCenter, fmt.Sprintf("azurerm_dev_center.test.id")},
"fleet_id": {dependencies.setNeedsKubernetesFleetManager, fmt.Sprintf("%s_kubernetes_fleet_manager.test.id", providerPrefix)},
Copy link
Member

Choose a reason for hiding this comment

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

fleet_id isn't consistent with how we refer to specific resource IDs, in the majority of cases we include the service in the property name. This list of dependencies is a good demonstration of that e.g. key_vault_id, key_vault_key_id, machine_learning_workspace_id etc.

So this needs to become

Suggested change
"fleet_id": {dependencies.setNeedsKubernetesFleetManager, fmt.Sprintf("%s_kubernetes_fleet_manager.test.id", providerPrefix)},
"kubernetes_fleet_id": {dependencies.setNeedsKubernetesFleetManager, fmt.Sprintf("%s_kubernetes_fleet_manager.test.id", providerPrefix)},

@ms-henglu
Copy link
Contributor Author

Hi @stephybun ,

About the review comment "changing fleet_id to kubernetes_fleet_id": #3416 (comment)

I did some tests and found that fleet_id is the generated field name and I can't override it to kubernetes_fleet_id by

        overrides "fleet_id" {
          updated_name = "kubernetes_fleet_id"
        }

I also checked the codes, it seems resource id fields doesn't support the overrides feature to update the name: https://github.com/hashicorp/pandora/blob/main/tools/importer-rest-api-specs/components/schema/fields_resource_id.go#L59

Please correct me if I was missing something. Thanks!

@ms-henglu
Copy link
Contributor Author

Thanks @stephybun for the quick fix! I've rebased the PR, please take another look.

@stephybun
Copy link
Member

I'd be happy to, but not all of the review points have been addressed, mainly point 2. and 3. in this comment at the very least 3. needs to be done otherwise the common ID won't be applied.

@ms-henglu
Copy link
Contributor Author

To address the point 2: hashicorp/go-azure-helpers#212

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @ms-henglu, I think this probably about ready. Let's merge and see what happens in the provider! LGTM 👍

@stephybun stephybun merged commit 865af7d into hashicorp:main Feb 6, 2024
3 checks passed
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