chore: Fix a batch of typos and a couple dead links across the repo#377
Open
dgreeninger-wandb wants to merge 7 commits into
Open
chore: Fix a batch of typos and a couple dead links across the repo#377dgreeninger-wandb wants to merge 7 commits into
dgreeninger-wandb wants to merge 7 commits into
Conversation
📝 WalkthroughWalkthroughThis PR makes non-functional edits: fixes typos and grammar, updates documentation links and phrasing, and adjusts several Terraform variable/output/ comment descriptions to reference the correct resource contexts (EKS, ALB, S3) without changing types, defaults, or logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@FAQ.md`:
- Line 22: Fix the grammar in the sentence containing "these list of resources";
change "these list of resources" to "this list of resources" and adjust the
surrounding phrasing for correct subject-verb agreement — e.g., rewrite the
fragment "to have a certain level of knowledge on this list of resources to be
able to deploy and maintain W&B successfully" to a clearer form such as "You
should have a certain level of knowledge of this list of resources to deploy and
maintain W&B successfully" (update the sentence in FAQ.md that contains the link
and the anchor "#aws-services-used").
In `@modules/database/variables.tf`:
- Around line 21-31: The variable declaration for create_db_subnet_group is
typed as string but uses a boolean default; change the variable's type to bool
(variable "create_db_subnet_group") so the default = true remains a boolean, and
then scan usages of var.create_db_subnet_group (e.g., conditional expressions or
resource count/for_each) to ensure they treat it as a boolean (remove any string
comparisons or quotes) so the module consistently uses a bool value.
In `@README.md`:
- Line 355: The heading text "Allow customer specific customer-managed keys for
S3 and RDS" contains a grammatical issue: change "customer specific" to the
hyphenated form "customer-specific" so the heading reads "Allow
customer-specific customer-managed keys for S3 and RDS" (update the exact
heading string in README.md).
- Around line 106-110: Update the README text to fix minor grammar: hyphenate
"cross-account" and capitalize "S3" where it currently reads "cross account" and
"s3" (in the explanatory text for bucket_permissions_mode /
bucket_restricted_accounts and the `public` option); keep all variable names
(`bucket_permissions_mode`, `bucket_restricted_accounts`, `BYOB`) unchanged.
- Line 21: The auto-generated Terraform requirements table in the README (inside
the BEGIN_TF_DOCS section) shows an incorrect Terraform constraint; regenerate
the tf-docs so the README reflects the actual required_version (~> 1.9) declared
in versions.tf. Update the documentation by running the docs generation step
(e.g., terraform-docs or your repo's doc generation script) that updates the
BEGIN_TF_DOCS block so the table shows required_version = "~> 1.9" and commit
the changed README.
Comment on lines
21
to
31
| variable "engine_version" { | ||
| description = "Version for MySQL Auora to use -- major version only" | ||
| description = "Version for MySQL Aurora to use -- major version only" | ||
| type = string | ||
| default = "8.0" | ||
| } | ||
|
|
||
| variable "create_db_subnet_group" { | ||
| description = "Determines whether to create the databae subnet group or use existing" | ||
| description = "Determines whether to create the database subnet group or use existing" | ||
| type = string | ||
| default = true | ||
| } |
There was a problem hiding this comment.
Pre-existing type mismatch on create_db_subnet_group: type = string with default = true.
default = true is a boolean literal, yet type = string is declared. Terraform silently coerces it to "true", but the variable semantically controls creation (on/off) and should be typed as bool.
🔧 Suggested fix
variable "create_db_subnet_group" {
description = "Determines whether to create the database subnet group or use existing"
- type = string
+ type = bool
default = true
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| variable "engine_version" { | |
| description = "Version for MySQL Auora to use -- major version only" | |
| description = "Version for MySQL Aurora to use -- major version only" | |
| type = string | |
| default = "8.0" | |
| } | |
| variable "create_db_subnet_group" { | |
| description = "Determines whether to create the databae subnet group or use existing" | |
| description = "Determines whether to create the database subnet group or use existing" | |
| type = string | |
| default = true | |
| } | |
| variable "engine_version" { | |
| description = "Version for MySQL Aurora to use -- major version only" | |
| type = string | |
| default = "8.0" | |
| } | |
| variable "create_db_subnet_group" { | |
| description = "Determines whether to create the database subnet group or use existing" | |
| type = bool | |
| default = true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/database/variables.tf` around lines 21 - 31, The variable declaration
for create_db_subnet_group is typed as string but uses a boolean default; change
the variable's type to bool (variable "create_db_subnet_group") so the default =
true remains a boolean, and then scan usages of var.create_db_subnet_group
(e.g., conditional expressions or resource count/for_each) to ensure they treat
it as a boolean (remove any string comparisons or quotes) so the module
consistently uses a bool value.
| however it does have the following pre-requisites: | ||
|
|
||
| ### Terrafom version >= 1.9 | ||
| ### Terraform version >= 1.9 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the Terraform required_version constraint
rg -n "required_version" --include="*.tf" -A1 -B1Repository: wandb/terraform-aws-wandb
Length of output: 510
🏁 Script executed:
# Check README.md at the mentioned lines
head -n 160 README.md | tail -n 140 | cat -nRepository: wandb/terraform-aws-wandb
Length of output: 7136
🏁 Script executed:
# Search for required_version in terraform files with correct syntax
rg "required_version" -t tf -A1 -B1Repository: wandb/terraform-aws-wandb
Length of output: 357
🏁 Script executed:
# Also check for any terraform blocks that might define this
rg "terraform\s*{" -t tf -A5Repository: wandb/terraform-aws-wandb
Length of output: 158
Update auto-generated requirements documentation to reflect the actual Terraform constraint.
The required_version in versions.tf is ~> 1.9, but the auto-generated requirements table in the README (line 129, within the BEGIN_TF_DOCS section) still shows ~> 1.0. This mismatch will mislead users about the module's Terraform version requirements. Regenerate the documentation to ensure it reflects the actual constraint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 21, The auto-generated Terraform requirements table in the
README (inside the BEGIN_TF_DOCS section) shows an incorrect Terraform
constraint; regenerate the tf-docs so the README reflects the actual
required_version (~> 1.9) declared in versions.tf. Update the documentation by
running the docs generation step (e.g., terraform-docs or your repo's doc
generation script) that updates the BEGIN_TF_DOCS block so the table shows
required_version = "~> 1.9" and commit the changed README.
Comment on lines
+106
to
+110
| We have added additional variables that make enabling BYOB easier. | ||
| `bucket_permissions_mode` accepts 1 of 3 values; | ||
| - `strict` the default requires an explict list of the buckets for proper access, the same as byob before `7.3.0`. | ||
| - `restricted` makes use of the new variable `bucket_restricted_accounts` which is a list of AWS account Id's where the BYOBs can be hosted from. ex: `["1234567890", "1234876590"]` | ||
| - `public` enables access to any BYOB properly configured not present in the the calling account. Effectively this enables cross account s3 access to ANY aws s3 account. | ||
| - `strict` the default requires an explicit list of the buckets for proper access, the same as byob before `7.3.0`. | ||
| - `restricted` makes use of the new variable `bucket_restricted_accounts` which is a list of AWS account IDs where the BYOBs can be hosted from. ex: `["1234567890", "1234876590"]` | ||
| - `public` enables access to any BYOB properly configured not present in the calling account. Effectively this enables cross account s3 access to ANY aws s3 account. |
There was a problem hiding this comment.
Minor grammar: "cross account" should be hyphenated as "cross-account" (line 110); "s3" should be capitalised "S3".
✏️ Suggested fix
-- `public` enables access to any BYOB properly configured not present in the calling account. Effectively this enables cross account s3 access to ANY aws s3 account.
+- `public` enables access to any BYOB properly configured not present in the calling account. Effectively this enables cross-account S3 access to ANY AWS S3 account.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| We have added additional variables that make enabling BYOB easier. | |
| `bucket_permissions_mode` accepts 1 of 3 values; | |
| - `strict` the default requires an explict list of the buckets for proper access, the same as byob before `7.3.0`. | |
| - `restricted` makes use of the new variable `bucket_restricted_accounts` which is a list of AWS account Id's where the BYOBs can be hosted from. ex: `["1234567890", "1234876590"]` | |
| - `public` enables access to any BYOB properly configured not present in the the calling account. Effectively this enables cross account s3 access to ANY aws s3 account. | |
| - `strict` the default requires an explicit list of the buckets for proper access, the same as byob before `7.3.0`. | |
| - `restricted` makes use of the new variable `bucket_restricted_accounts` which is a list of AWS account IDs where the BYOBs can be hosted from. ex: `["1234567890", "1234876590"]` | |
| - `public` enables access to any BYOB properly configured not present in the calling account. Effectively this enables cross account s3 access to ANY aws s3 account. | |
| We have added additional variables that make enabling BYOB easier. | |
| `bucket_permissions_mode` accepts 1 of 3 values; | |
| - `strict` the default requires an explicit list of the buckets for proper access, the same as byob before `7.3.0`. | |
| - `restricted` makes use of the new variable `bucket_restricted_accounts` which is a list of AWS account IDs where the BYOBs can be hosted from. ex: `["1234567890", "1234876590"]` | |
| - `public` enables access to any BYOB properly configured not present in the calling account. Effectively this enables cross-account S3 access to ANY AWS S3 account. |
🧰 Tools
🪛 LanguageTool
[grammar] ~110-~110: Use a hyphen to join words.
Context: ... account. Effectively this enables cross account s3 access to ANY aws s3 account....
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 106 - 110, Update the README text to fix minor
grammar: hyphenate "cross-account" and capitalize "S3" where it currently reads
"cross account" and "s3" (in the explanatory text for bucket_permissions_mode /
bucket_restricted_accounts and the `public` option); keep all variable names
(`bucket_permissions_mode`, `bucket_restricted_accounts`, `BYOB`) unchanged.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 113: Update the sentence in the README that reads "Enabling BYOB or
cross-account regardless of `bucket_permissions_mode` still requires a policy
attached to that bucket allowing the eks node role to perform s3 actions." to
capitalize the acronyms: change "eks" to "EKS" and "s3" to "S3" so it reads
"...allowing the EKS node role to perform S3 actions." Ensure the backticked
`bucket_permissions_mode` remains unchanged.
---
Duplicate comments:
In `@README.md`:
- Line 110: Update the modified README line to hyphenate "cross-account" and
capitalize AWS service names: replace the phrase "cross account s3" with
"cross-account S3" and change "aws s3" to "AWS S3" (and ensure any other
occurrences on the same line match this capitalization), e.g., in the sentence
that currently reads "`public` enables access to any BYOB properly configured
not present in the calling account. Effectively this enables cross account s3
access to ANY aws s3 account." update those tokens accordingly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Noticed a few typos in the README, so decided to run through the entire module.
Summary by CodeRabbit