Skip to content

Conversation

@bibek4699
Copy link
Contributor

@bibek4699 bibek4699 commented Jan 31, 2023

Please confirm that you have done the following before requesting reviews:

  • I have confirmed that the PR type is appropriate for the change I am making according to the Honest Pull Request and Commit Message Naming Conventions.
  • I have typed an adequate description that explains why I am making this change.
  • I have installed and run standard pre-commit hooks that lints and validates my code.

Description

Provision network and peering component for confluent Kafka
Terratest file to test the component


This change is Reviewable

@bibek4699 bibek4699 requested a review from a team as a code owner January 31, 2023 18:41
@bibek4699 bibek4699 requested a review from jai January 31, 2023 18:41
@bibek4699 bibek4699 changed the title DRAFT: feat: Create dedicated network component for confluent kafka [DEVOP-2387] feat: Create dedicated network component for confluent kafka [DEVOP-2387] Feb 2, 2023
@@ -0,0 +1,44 @@
<!-- BEGIN_TF_DOCS -->
Copy link
Contributor

Choose a reason for hiding this comment

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

title/please describe the module

}

output "confluent_resource_name" {
description = "The Confluent Resource Name of the Network."
Copy link
Contributor

Choose a reason for hiding this comment

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

please standardize whether descriptions have/don't have punctuations

@@ -0,0 +1,28 @@
resource "random_id" "suffix" {
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a README to this folder

Copy link

@MXfive MXfive left a comment

Choose a reason for hiding this comment

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

Just saw my comments from the other week were still pending.

@@ -0,0 +1,10 @@
terraform {
required_version = ">= 0.13"
Copy link

Choose a reason for hiding this comment

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

Suggested change
required_version = ">= 0.13"
required_version = ">= 1.3"

@@ -0,0 +1,3 @@
module examples/confluent-dedicated-cluster
Copy link

Choose a reason for hiding this comment

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

Why do we have this in the examples folder?

Comment on lines +30 to +31
default = "PEERING"
validation {
Copy link

Choose a reason for hiding this comment

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

Suggested change
default = "PEERING"
validation {
default = "PEERING"
validation {

Comment on lines +39 to +40
type = string
validation {
Copy link

Choose a reason for hiding this comment

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

Suggested change
type = string
validation {
type = string
validation {

Comment on lines +31 to +41
display_name = var.confluent_network_peering_name
gcp {
project = var.gcp_project_id
vpc_network = var.gcp_vpc_network
#customer_region = var.region
import_custom_routes = var.import_custom_routes
}
environment {
id = var.environment_id
}
network {
Copy link

Choose a reason for hiding this comment

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

Suggested change
display_name = var.confluent_network_peering_name
gcp {
project = var.gcp_project_id
vpc_network = var.gcp_vpc_network
#customer_region = var.region
import_custom_routes = var.import_custom_routes
}
environment {
id = var.environment_id
}
network {
display_name = var.confluent_network_peering_name
gcp {
project = var.gcp_project_id
vpc_network = var.gcp_vpc_network
#customer_region = var.region
import_custom_routes = var.import_custom_routes
}
environment {
id = var.environment_id
}
network {

Why the commented out customer_region ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants