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

docs: Update and standardize examples to follow conventions defined in pre-commit checks #424

Merged
merged 16 commits into from
Apr 29, 2022
Merged

docs: Update and standardize examples to follow conventions defined in pre-commit checks #424

merged 16 commits into from
Apr 29, 2022

Conversation

bryantbiggs
Copy link
Contributor

@bryantbiggs bryantbiggs commented Apr 19, 2022

What does this PR do?

  • Standardize AWS provider region usage within examples; remove use of variable for region in provider since this should be static
  • Remove references to prior license material and MIT licensing
  • Standardize provider and required versions within versions.tf per conventions; this also aligns versions that are used to be consistent
  • Update pre-commit checks
    • Update versions to use latest
    • Remove .tflint.hcl file that wasn't enforcing lint rules, add rules to pre-commit to ensure conventions are enforced
    • Update terraform_docs to ensure lockfile doesn't affect version shown in READMEs
  • Update and align version used on VPC module within examples
  • Move outputs to outputs.tf file per convention; add empty file where one is not present in the directory
  • Correct tflint errors
    • Remove unused locals and data sources
    • Ensure variables and outputs have descriptions
    • Ensure names use underscores instead of hyphens
    • The managed node group policy attachment resource names are left as is with a TODO for now since that is a breaking change
    • Ensure provider versions are set in required providers
    • Quite a few whitepsace changes
  • Correct terraform_docs replacement pattern so that pre-commit will update documentation on detected changes; this was not updating previously and the documentation was out of sync with the source
  • Update kubernetes, helm, and kubectl providers to use exec() method per the Terraform provider docs. This follows same pattern used by the EKS service within the kubeconfig file to ensure credentials are refreshed. This also removes the use of two data sources aws_eks_cluster_auth and aws_eks_cluster per example

Motivation

More

  • Yes, I have tested the PR using my local account setup (Provide any test evidence report under Additional Notes)
  • Yes, I have added a new example under examples to support my PR
  • Yes, I have created another PR for add-ons under add-ons repo (if applicable)
  • Yes, I have updated the docs for this feature
  • Yes, I ran pre-commit run -a with this PR

Note: Not all the PRs required examples and docs except a new pattern or add-on added.

For Moderators

  • E2E Test successfully complete before merge?

Additional Notes

@@ -45,5 +17,4 @@ module "eks-cluster-with-import-vpc" {

vpc_id = data.terraform_remote_state.vpc_s3_backend.outputs.vpc_id
private_subnet_ids = data.terraform_remote_state.vpc_s3_backend.outputs.private_subnets
public_subnet_ids = data.terraform_remote_state.vpc_s3_backend.outputs.public_subnets
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Terraform validate gave this error when it was present:

│ Error: Unsupported argument
│ 
│   on main.tf line 20, in module "eks_cluster_with_import_vpc":
│   20:   public_subnet_ids  = data.terraform_remote_state.vpc_s3_backend.outputs.public_subnets
│ 
│ An argument named "public_subnet_ids" is not expected here.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @vara-bonthu , do we still need PR, are we still using pr-test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, you can remove the deploy/pr folder and pr-test.yaml github workflow. We can replace this with examples-tfplan-tests.yaml github workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we still need the vpc-test.yml and deploy/e2e/ as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

No we don't need that as well. We may need to cleanup the existing VPC by running vpc-test-cleanup.yaml workflow before removing the following files

vpc-test.yaml , vpc-test-cleanup.yaml e2e-test.yaml and e2e-test-cleanup.yaml . All these will be replaced with Terratest e2e-terratest.yaml

deploy/e2e/eks and deploy/e2e/vpc can be removed which is used by the above workflows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in 4379d40 - @Zvikan if you wouldn't mind taking a look at that commit to make sure everything aligns with your expectations

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, thank you @bryantbiggs

@bryantbiggs bryantbiggs mentioned this pull request Apr 19, 2022
6 tasks
Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

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

LGTM! I have added few questions.

You also added few empty outputs.tf files for few add-ons. Is this for supporting pre-commit checks?

deploy/pr/versions.tf Outdated Show resolved Hide resolved
examples/analytics/emr-on-eks/main.tf Show resolved Hide resolved
modules/kubernetes-addons/agones/versions.tf Show resolved Hide resolved
@bryantbiggs
Copy link
Contributor Author

regarding the versions and outputs - because we are stating that these are modules then the tflint rules apply; specifically:

  • terraform_standard_module_structure
  • terraform_required_providers
  • terraform_required_version

if you copy the .pre-commit-config.yaml file from this PR and execute it locally, you can see the errors that show up from tflint

Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

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

LGTM! E2E tests complete?
It's worth creating a tag before merging this PR

@kcoleman731 kcoleman731 merged commit deec7d5 into aws-ia:main Apr 29, 2022
@bryantbiggs bryantbiggs deleted the docs/example-locals branch April 29, 2022 21:37
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.

[QUESTION] Licence MIT-0 vs. Apache 2.0
4 participants