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

azurerm_hdinsight_kafka_cluster: support rest proxy #8064

Merged

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Aug 10, 2020

This PR add supports for kafka rest proxy. To use kafka rest proxy, user will needs to specify both the rest prox setting and also a "kafka management node". In return, we will need to expose the endpoint for the kafka rest proxy.

Fixes #8053

@ghost ghost added the size/L label Aug 10, 2020
@patelprakashp

This comment has been minimized.

@ghost ghost added documentation size/XL and removed size/L labels Aug 11, 2020
@magodo magodo marked this pull request as ready for review August 11, 2020 03:19
@patelprakashp

This comment has been minimized.

Copy link
Contributor

@kosinsky kosinsky left a comment

Choose a reason for hiding this comment

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

Looks good for me.

@patelprakashp

This comment has been minimized.

@katbyte katbyte added this to the v2.23.0 milestone Aug 12, 2020
@tombuildsstuff tombuildsstuff modified the milestones: v2.23.0, v2.24.0 Aug 13, 2020
@tombuildsstuff tombuildsstuff removed this from the v2.24.0 milestone Aug 20, 2020
@tombuildsstuff
Copy link
Member

@magodo can you rebase this PR?

@patelprakashp

This comment has been minimized.

@magodo
Copy link
Collaborator Author

magodo commented Aug 21, 2020

@tombuildsstuff I merged the master into this branch as that is easier to resolve the conflict.

@patelprakashp

This comment has been minimized.

@patelprakashp

This comment has been minimized.

@tombuildsstuff
Copy link
Member

@patelprakashp respectfully, would you mind refraining from @-ing people asking when this’ll be merged?

Whilst I appreciate you’d like to see this feature merged, unfortunately each notification is more noise for maintainers, which ultimately means that things take longer to get reviewed since we have more noise to filter out - we instead ask that you use reactions (👍🏻) on the Issue/PR so that we can gauge interest. For the moment I’ve marked the “when will this be merged” comments as off-topic.

Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @magodo

Thanks for this PR / rebasing this.

Taking a look through this is looking good - if we can fix up the comments here then we should be able to take another look 👍

Thanks!

HeadNodeDef: hdInsightKafkaClusterHeadNodeDefinition,
WorkerNodeDef: hdInsightKafkaClusterWorkerNodeDefinition,
ZookeeperNodeDef: hdInsightKafkaClusterZookeeperNodeDefinition,
KafkaManagementNodeDef: &hdInsightKafkaClusterKafkaManagementNodeDefinition,
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be conditionally set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should do no harm (and I refer to the same way which has been done for the EdgeNodeDef). Because when you do expand, it will further check whether user has specify the corresponding schema; when you do flatten, it will further find the corresponding property by name (in this case kafkamanagementnode) in the response body. In both cases, if the kafka management node is not enabled, it shall do nothing.

website/docs/r/hdinsight_kafka_cluster.html.markdown Outdated Show resolved Hide resolved
@magodo
Copy link
Collaborator Author

magodo commented Sep 2, 2020

@tombuildsstuff

I have made the change that use aad client to get the group name instead of asking user to provide. Please take another look!

Test Result

💤 make testacc TEST=./azurerm/internal/services/hdinsight/tests TESTARGS='-run TestAccAzureRMHDInsightKafkaCluster_restProxy'
                                                                                                                                                              
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...                                                                                                        
TF_ACC=1 go test ./azurerm/internal/services/hdinsight/tests -v -run TestAccAzureRMHDInsightKafkaCluster_restProxy -timeout 180m -ldflags="-X=github.com/terra
form-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMHDInsightKafkaCluster_restProxy                                                                                                       
=== PAUSE TestAccAzureRMHDInsightKafkaCluster_restProxy                                                                                                       
=== CONT  TestAccAzureRMHDInsightKafkaCluster_restProxy
--- PASS: TestAccAzureRMHDInsightKafkaCluster_restProxy (1502.83s)                                                                                            
PASS   
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/hdinsight/tests     1502.859s

@patelprakashp

This comment has been minimized.

@ghost ghost removed the waiting-response label Sep 16, 2020
@katbyte
Copy link
Collaborator

katbyte commented Dec 30, 2020

@magodo - could we get this rebased? should be good to merge once thats done

@magodo
Copy link
Collaborator Author

magodo commented Dec 30, 2020

@katbyte I've resolved the conflicts, please take another look!

💤 make testacc TEST=./azurerm/internal/services/hdinsight TESTARGS='-run "TestAccHDInsightKafkaCluster_restProxy|TestAccDataSourceHDInsightCluster_kafkaWithRestProxy"'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/hdinsight -v -run "TestAccHDInsightKafkaCluster_restProxy|TestAccDataSourceHDInsightCluster_kafkaWithRestProxy" -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccDataSourceHDInsightCluster_kafkaWithRestProxy
=== PAUSE TestAccDataSourceHDInsightCluster_kafkaWithRestProxy
=== RUN   TestAccHDInsightKafkaCluster_restProxy
=== PAUSE TestAccHDInsightKafkaCluster_restProxy
=== CONT  TestAccDataSourceHDInsightCluster_kafkaWithRestProxy
=== CONT  TestAccHDInsightKafkaCluster_restProxy
--- PASS: TestAccDataSourceHDInsightCluster_kafkaWithRestProxy (1156.12s)
--- PASS: TestAccHDInsightKafkaCluster_restProxy (1168.85s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/hdinsight   1168.897s

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @magodo - this LGTM 👍

@katbyte katbyte added this to the v2.42.0 milestone Jan 5, 2021
@katbyte katbyte merged commit 4c83bce into hashicorp:master Jan 5, 2021
katbyte added a commit that referenced this pull request Jan 5, 2021
katbyte added a commit that referenced this pull request Jan 5, 2021
@ghost
Copy link

ghost commented Jan 8, 2021

This has been released in version 2.42.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.42.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Feb 5, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Feb 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Kafka-Rest proxy provisioning on HDinsight Kafka Cluster
5 participants