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
Enable cluster_admin in Kubernetes service connections #218
Conversation
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.
@Chathuru Looks good for your first PR. A few changes requested.
You need add an acceptance test(Terraform Acceptance Test) for the new property in resource_serviceendpoint_kubernetes_test.go. We use acceptance to ensure that the provider is work correct.
An example acceptance test:
var config = `
resource azuredevops_project "project" {
name = "k8s-test"
description = "Test Project Description"
visibility = "private"
version_control = "Git"
work_item_template = "Agile"
features = {
"testplans" = "disabled"
"artifacts" = "disabled"
}
}
resource "azuredevops_serviceendpoint_kubernetes" "serviceendpoint" {
project_id = azuredevops_project.project.id
service_endpoint_name = "k8s-test-ado"
apiserver_url = "https://sample-kubernetes-cluster.hcp.westeurope.azmk8s.io"
authorization_type = "AzureSubscription"
// description = "managed by terraform"
azure_subscription {
subscription_id = "00000000-0000-0000-0000-000000000000"
subscription_name = "Test Subscription Name"
tenant_id = "00000000-0000-0000-0000-000000000000"
resourcegroup_id = "Test-Resource-Group"
namespace = "default"
cluster_name = "TestClusterName"
cluster_admin = true
}
}`
azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_kubernetes.go
Show resolved
Hide resolved
azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_kubernetes.go
Outdated
Show resolved
Hide resolved
@@ -255,6 +262,7 @@ func flattenServiceEndpointKubernetes(d *schema.ResourceData, serviceEndpoint *s | |||
clusterNameIndex = k + 1 | |||
} | |||
} | |||
clusterAdmin, _ := strconv.ParseBool((*serviceEndpoint.Data)["cluster_admin"]) |
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.
cluster_admin
should be changed toclusterAdmin
, this is the property name returned by service.- You need handle the error return by
strconv.ParseBool(arg1, arg2)
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.
Use
fmt.Errorfto handel
strconv.ParseBool(arg1, arg2)` error. Is this the correct way to handle errors?- Don’t we need to handle errors in https://github.com/Chathuru/terraform-provider-azuredevops/blob/master/azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_kubernetes.go#L291 as well?
Still working on the acceptance test
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.
You are correct. The error
should be handled when strconv.ParseBool
working incorrect. https://github.com/Chathuru/terraform-provider-azuredevops/blob/master/azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_kubernetes.go#L291
LGTM |
All Submissions:
What about the current behavior has changed?
Allow to enable and disable cluster_admin in Kubernetes service connections with Azure subscription authentication type.
Issue Number: #216
Does this introduce a change to
go.mod
,go.sum
orvendor/
?Does this introduce a breaking change?
Any relevant logs, error output, etc?
(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)
Other information