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

add module naming convention #12

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add module naming convention #12

wants to merge 1 commit into from

Conversation

migara
Copy link
Member

@migara migara commented May 26, 2021

Description

Documentation update

## Module naming convention
We will use [kebab-case](https://www.theserverside.com/definition/Kebab-case) when naming the modules

Examples
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Examples
Examples:

transit-gateway
gateway-loadbalancer
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Module structure:

@damianfedeczko
Copy link
Contributor

Veeery minor suggestions + linter failed 🥳

@@ -453,6 +453,15 @@ proposes workarounds. The 0.14 version is not impacted by this issue.

We will maintain one mono repo per cloud provider. Three repos, representing AWS, Azure and GCP, as we kick off our efforts.

## Module naming convention
We will use [kebab-case](https://www.theserverside.com/definition/Kebab-case) when naming the modules
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be against. We have so many modules named with snake_case. Renaming all of them is a big change, so I'd wonder what strong arguments are there?

ls ../terraform-*/modules
../terraform-aws-vmseries-modules/modules:
asg  bootstrap  crosszone_failover  gwlb  panorama  transit_gateway  vmseries  vpc  vpc_endpoint  vpc_routes

../terraform-google-vmseries-modules/modules:
autoscale  bootstrap  gcp_bootstrap  iam_service_account  lb_http_ext_global  lb_tcp_external  lb_tcp_internal  panorama  vm  vmseries  vpc

When programming, there is a temptation to first do use the identical string:

module "security-group" { # <-- copy pasted from `source`
  source  = "terraform-aws-modules/security-group/aws"
  version = "4.0.0"
}

This either fails the linting rule after some time, or if uncaught, it leads to this painful experience:

id = module.security-group.security_group.sg_id

Subjectively I know the excerpts like the previous line hurt my productivity a lot. But I know it won't be the same for all of us.

(Introducing a long-existing inconsistency is worse. In usage, if a dev or user is always in doubt whether this time it's transit-gateway or transit_gateway, that sums up over all these people to quite a noticeable problem.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the name of the main module, rather than changing every variable name. I could be wrong though. Care to comment @migara ?

@devsecfranklin
Copy link
Contributor

Got a few linter errors it looks like:

README.md: 456: MD022/blanks-around-headings/blanks-around-headers Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "## Module naming convention"]
README.md: 460: MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```"]
README.md: 460: MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"]
README.md: 460: MD046/code-block-style Code block style [Expected: indented; Actual: fenced]

Copy link
Contributor

@devsecfranklin devsecfranklin left a comment

Choose a reason for hiding this comment

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

few linter errors but LGTM overall

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.

4 participants