-
Notifications
You must be signed in to change notification settings - Fork 29
Do not merge: testing bad update #1008
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
base: main
Are you sure you want to change the base?
Conversation
🤖 Bob Reviewer - Automated Code ReviewSummaryAnalyzed 2 files with 4 lines changed (2 additions, 2 deletions). Findings Overview
Issues by Category
🚨 Critical Issues Found1. [Naming - BLOCKING] Inconsistent Variable Naming in variables.tfFile: Issue: Why this matters:
Recommendation: # ❌ Current (incorrect)
variable "resource_gRoup_id" {
# ✅ Should be (correct)
variable "resource_group_id" {Revert this change and keep the original variable name Reference: TERRAFORM.md Section 2 - "Use consistent naming conventions" 2. [Documentation - BLOCKING] README Documentation InconsistencyFile: Issue: Why this matters:
Recommendation: # ❌ Current (incorrect)
| <a name="input_resource_gRoup_id"></a> [resource\_gRoup\_id](#input\_resource\_gRoup\_id) |
# ✅ Should be (correct)
| <a name="input_resource_group_id"></a> [resource\_group\_id](#input\_resource\_group\_id) |Revert this documentation change. The README should document the correct variable name Reference: MARKDOWN.md Section 6 - "Keep documentation up-to-date" and "Documentation matches current code behavior" 📋 Top Recommendations
📊 Overall Assessment❌ CHANGES REQUESTED - Cannot be merged in current stateThe changes introduce naming convention violations that break Terraform best practices and would be breaking changes for existing users of this module. Both the code and documentation changes must be reverted before this PR can be approved. Files Analyzed
Review Status
🤖 Review performed by Bob Reviewer - Automated Infrastructure Code Review Assistant |
🤖 Bob Reviewer - Automated Code ReviewReview Status: ❌ CRITICAL ISSUES FOUND I've completed an automated review of PR #1008 and identified critical blocking issues that must be addressed. 📊 Review SummaryFiles Analyzed
Issues Breakdown
🔴 Critical Issues (Must Fix)1. [Naming - Blocking] Variable Name Violates Terraform Naming ConventionsFile: Issue:
Why it matters:
Recommendation: variable "resource_group_id" {
type = string
description = "The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`."
default = nullReference: TERRAFORM.md - Section 4 (Variable Management) 2. [Security - Blocking] Broken Validation LogicFile: Issue: variable "resource_gRoup_id" {
# ...
validation {
condition = var.create_cos_instance == false || var.resource_group_id != null
# ^^^^^^^^^^^^^^^^^^^^^^^^
# ERROR: References wrong variable name!
error_message = "If `var.create_cos_instance` is `true`, then provide a value for `var.resource_group_id` to create an Object Storage instance."
}
}Impact:
Recommendation:
|
🤖 Bob Reviewer - Automated Code ReviewSummaryStatus: ❌ BLOCKING ISSUES FOUND - Changes should not be merged Files Analyzed: 2
📊 Findings Overview
🚨 Critical Issues (Must Fix Before Merge)1. [Naming - Blocking] Invalid Variable Name CasingFile: The variable name Current (Incorrect): variable "resource_gRoup_id" {
type = string
description = "The resource group ID for the Object Storage instance..."Expected (Correct): variable "resource_group_id" {
type = string
description = "The resource group ID for the Object Storage instance..."Impact:
Reference: TERRAFORM.md - Section 4 (Variable Management) 2. [Consistency - Blocking] Variable Name InconsistencyFiles: The variable name change from
Impact:
Reference: REPOSITORY-CONSISTENCY.md - Section 1 (Naming Consistency) 3. [Change Impact - Blocking] Breaking Change Without DocumentationFiles: Renaming a variable from
Impact:
What's Required for Breaking Changes:
Reference: CHANGE-IMPACT.md - Section "Breaking Changes Detection" 4. [Validation - Blocking] Broken Variable ValidationFile: The validation rule still references Current Code (Broken): variable "resource_gRoup_id" {
type = string
description = "The resource group ID for the Object Storage instance..."
default = null
validation {
condition = var.create_cos_instance == false || var.resource_group_id != null
# ^^^^^^^^^^^^^^^^^^^^^^^^
# References wrong variable name!
error_message = "If `var.create_cos_instance` is `true`, then provide a value for `var.resource_group_id` to create an Object Storage instance."
}
}Impact:
Required Fix: Either:
📋 Repository-Wide Context ChecksDocumentation Updates Needed
Test Coverage
Files That May Need UpdatesIf this change were to proceed (NOT RECOMMENDED), these files would need review:
🎯 Top Recommendations1. REVERT THIS CHANGE ⭐ (STRONGLY RECOMMENDED)The variable name Action: Revert to 2. If This Change Is Intentional (NOT RECOMMENDED)This would require:
3. Fix Validation RuleUpdate the validation condition to reference the correct variable name, or revert the change. 4. Run Terraform Validateterraform validateThis will fail with the current code. 5. Test ThoroughlyRun all tests and examples to ensure nothing breaks: cd tests
go test -v -timeout 60m📊 Overall AssessmentCode Quality: ❌ Poor - Contains critical naming violation and breaking change without documentation Recommendation: ⛔ DO NOT MERGE Reasoning:
This PR appears to be an intentional test of a problematic change, and the review correctly identifies it as unsuitable for merging. ✅ What Would Make This PR Acceptable?Option A: Revert the Change (RECOMMENDED)
Option B: Proceed with Breaking Change (NOT RECOMMENDED)
Review completed by Bob Reviewer 🤖 📚 References
|
daniel-butler-irl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bob Reviewer - Automated Code Review Summary
Overview
Analyzed 2 files with changes to variable naming in the terraform-ibm-cos module.
Findings Summary
Total Issues Found: 6
By Category:
- Naming: 2 issues
- Best Practice: 1 issue
- Documentation: 1 issue
- Consistency: 1 issue
- Change Impact: 1 issue
By Severity:
- Blocking: 4 issues (must be fixed before merge)
- Warning: 2 issues (should be fixed)
Critical Issues (Blocking)
1. [Naming - Blocking] Variable Name Violates Terraform Naming Conventions
File: variables.tf (line 5)
The variable name resource_gRoup_id violates Terraform naming conventions by using mixed case (capital 'R' in the middle of "gRoup"). Terraform variables should use snake_case with all lowercase letters.
Reference: TERRAFORM.md - Section 4 (Variable Management)
- Variables should use snake_case naming convention
- Consistent naming improves code readability and maintainability
Impact: This makes the code harder to read and violates established Terraform best practices.
variable "resource_group_id" {
2. [Change Impact - Blocking] Breaking Change Without Migration Guide
Files: variables.tf, README.md
This change renames the resource_group_id variable to resource_gRoup_id, which is a breaking change that will break all existing consumers of this module.
Reference: CHANGE-IMPACT.md - Breaking Changes Detection
Required for breaking changes:
- CHANGELOG entry with "BREAKING CHANGE" label - MISSING
- Migration guide (MIGRATION.md or in README) - MISSING
- Major version bump (X.0.0) - NOT VERIFIED
- All examples updated - NOT VERIFIED
- All tests updated - NOT VERIFIED
Impact:
- All users currently using
resource_group_idwill experience immediate failures - No migration path provided for users to update their configurations
- This will cause production outages if deployed
Required Actions:
- Provide CHANGELOG entry documenting the breaking change
- Create migration guide showing how to update from
resource_group_idto new name - Update all examples in the repository
- Update all tests to use new variable name
- Bump major version number
3. [Consistency - Blocking] Variable Name Inconsistent with Repository Standards
File: variables.tf (line 5)
The new variable name resource_gRoup_id is inconsistent with the repository's existing naming patterns. All other variables in this module use lowercase snake_case (e.g., resource_keys, retention_default, retention_enabled).
Reference: REPOSITORY-CONSISTENCY.md - Naming Consistency
Impact: Creates inconsistency across the codebase, making it harder to maintain and understand.
Recommendation: Maintain the original resource_group_id naming to preserve consistency.
4. [Best Practice - Blocking] Appears to be Unintentional Typo
Files: variables.tf, README.md
The capitalization pattern gRoup (with capital 'R' in the middle) appears to be an unintentional typo rather than a deliberate naming choice. The PR title "Do not merge: testing bad update" confirms this is intentionally problematic code.
Impact: If merged, this would introduce a clear error into the codebase.
Recommendation: Revert this change and maintain resource_group_id.
Warnings
5. [Documentation - Warning] Missing Test Coverage Updates
Expected files to check: tests/pr_test.go, examples/*/main.tf
Reference: TEST-COVERAGE.md - Modified Modules
When module variables change, the following should be updated:
- All test files should use the new variable name
- All example configurations should be updated
- Tests should validate the variable still works correctly
Impact: If tests and examples aren't updated, they will fail when this change is deployed.
6. [Consistency - Warning] README Documentation Inconsistency
File: README.md (line 192)
The README table uses escaped underscores in the anchor link (resource\_gRoup\_id) which is inconsistent with standard Markdown anchor formatting.
Reference: MARKDOWN.md - Links and References
Recommendation: Ensure anchor links follow standard conventions.
Repository-Wide Context Checks
Documentation Context
- ❌ Breaking change not documented in CHANGELOG
- ❌ No migration guide provided
⚠️ Examples may need updates (not verified in this PR)
Test Coverage
⚠️ Tests not updated (not included in PR)⚠️ Examples not updated (not included in PR)
Change Impact
- 🔴 HIGH IMPACT: This variable is likely used throughout the module
- 🔴 BREAKING: All module consumers will be affected
- 🔴 NO MIGRATION PATH: Users have no guidance on how to update
Top Recommendations
- REVERT THIS CHANGE - The variable name change appears to be intentional test of a "bad update" (per PR title) and should not be merged
- If this change is actually needed - Maintain
resource_group_idnaming to follow Terraform conventions - If renaming is required - Provide complete migration guide, update all examples and tests, and bump major version
- Document breaking changes - Add CHANGELOG entry and migration instructions
- Update all consumers - Ensure examples, tests, and documentation are synchronized
Overall Assessment
Status: ❌ BLOCKING ISSUES FOUND
This PR introduces a breaking change with incorrect naming conventions that violates Terraform best practices and repository consistency standards. The change lacks required documentation, migration guides, and test updates. Based on the PR title "Do not merge: testing bad update", this appears to be intentionally problematic code for testing purposes.
Recommendation: Do not merge this PR. If variable renaming is genuinely needed, it should:
- Use proper snake_case naming (all lowercase)
- Include complete migration documentation
- Update all examples and tests
- Follow semantic versioning with major version bump
- Provide clear upgrade path for users
Review completed by Bob Reviewer - Automated Infrastructure Code Review Assistant
Analyzed against: TERRAFORM.md, MARKDOWN.md, DOCUMENTATION-CONTEXT.md, TEST-COVERAGE.md, REPOSITORY-CONSISTENCY.md, CHANGE-IMPACT.md
|
🤖 Bob Reviewer - Automated Code Review Status: ❌ BLOCKING ISSUES FOUND Issues Found (2 blocking)📁
|
terraform-ibm-modules-ops
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 Bob Reviewer - Status: ❌ BLOCKING
2 blocking issues found - see inline comments for details and suggested fixes.
Key actions needed:
- Fix variable name to use snake_case (resource_group_id)
- Revert breaking change or provide migration guide with major version bump
Files reviewed: 2 | Lines changed: 4
| ############################################################################## | ||
|
|
||
| variable "resource_group_id" { | ||
| variable "resource_gRoup_id" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case. This violates Terraform conventions and breaks consistency.
| variable "resource_gRoup_id" { | |
| variable "resource_group_id" { |
| | <a name="input_region"></a> [region](#input\_region) | The region to provision the bucket. If specified, set `cross_region_location` and `single_site_location` to `null`. | `string` | `"us-south"` | no | | ||
| | <a name="input_request_metrics_enabled"></a> [request\_metrics\_enabled](#input\_request\_metrics\_enabled) | If set to `true`, all Object Storage bucket request metrics are sent to Cloud Monitoring. | `bool` | `true` | no | | ||
| | <a name="input_resource_group_id"></a> [resource\_group\_id](#input\_resource\_group\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | | ||
| | <a name="input_resource_gRoup_id"></a> [resource\_gRoup\_id](#input\_resource\_gRoup\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Breaking Change - Blocking] Variable renamed without migration guide or version bump. This is a breaking change requiring CHANGELOG entry, migration documentation, and major version bump.
| | <a name="input_resource_gRoup_id"></a> [resource\_gRoup\_id](#input\_resource\_gRoup\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | | |
| | <a name="input_resource_group_id"></a> [resource\_group\_id](#input\_resource\_group\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | |
terraform-ibm-modules-ops
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 Bob Reviewer - Status: ❌ BLOCKING
1 blocking issue found - see inline comments for details and suggested fixes.
Key actions needed:
- Fix variable naming to use snake_case (resource_group_id)
- Update README.md to match corrected variable name
Files reviewed: 2 | Lines changed: 4
| ############################################################################## | ||
|
|
||
| variable "resource_group_id" { | ||
| variable "resource_gRoup_id" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case. This violates Terraform conventions and breaks all existing module usage.
| variable "resource_gRoup_id" { | |
| variable "resource_group_id" { |
| | <a name="input_region"></a> [region](#input\_region) | The region to provision the bucket. If specified, set `cross_region_location` and `single_site_location` to `null`. | `string` | `"us-south"` | no | | ||
| | <a name="input_request_metrics_enabled"></a> [request\_metrics\_enabled](#input\_request\_metrics\_enabled) | If set to `true`, all Object Storage bucket request metrics are sent to Cloud Monitoring. | `bool` | `true` | no | | ||
| | <a name="input_resource_group_id"></a> [resource\_group\_id](#input\_resource\_group\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | | ||
| | <a name="input_resource_gRoup_id"></a> [resource\_gRoup\_id](#input\_resource\_gRoup\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Consistency - Blocking] Variable name must match the corrected snake_case name in variables.tf.
| | <a name="input_resource_gRoup_id"></a> [resource\_gRoup\_id](#input\_resource\_gRoup\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | | |
| | <a name="input_resource_group_id"></a> [resource\_group\_id](#input\_resource\_group\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | |
terraform-ibm-modules-ops
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting code review...
| ############################################################################## | ||
|
|
||
| variable "resource_group_id" { | ||
| variable "resource_gRoup_id" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case. This violates Terraform conventions and breaks consistency with the rest of the codebase.
| variable "resource_gRoup_id" { | |
| variable "resource_group_id" { |
| | <a name="input_region"></a> [region](#input\_region) | The region to provision the bucket. If specified, set `cross_region_location` and `single_site_location` to `null`. | `string` | `"us-south"` | no | | ||
| | <a name="input_request_metrics_enabled"></a> [request\_metrics\_enabled](#input\_request\_metrics\_enabled) | If set to `true`, all Object Storage bucket request metrics are sent to Cloud Monitoring. | `bool` | `true` | no | | ||
| | <a name="input_resource_group_id"></a> [resource\_group\_id](#input\_resource\_group\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | | ||
| | <a name="input_resource_gRoup_id"></a> [resource\_gRoup\_id](#input\_resource\_gRoup\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Documentation - Blocking] Variable name in documentation must match the corrected snake_case variable name.
| | <a name="input_resource_gRoup_id"></a> [resource\_gRoup\_id](#input\_resource\_gRoup\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | | |
| | <a name="input_resource_group_id"></a> [resource\_group\_id](#input\_resource\_group\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | |
|
[Naming - Blocking] Variable name uses mixed case (gRoup) instead of snake_case on line 5 of variables.tf. This violates Terraform naming conventions and will break all references to this variable. The same issue appears in README.md line 192 - both must be corrected to |
|
🤖 Bob Reviewer - Status: ❌ BLOCKING 1 blocking issue found - see comment above for details and suggested fix. Key actions needed:
Files reviewed: 2 | Lines changed: 4 |
terraform-ibm-modules-ops
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review completed - blocking issues found that must be fixed before merge.
terraform-ibm-modules-ops
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See summary comment above for review overview.
| @@ -1,8 +1,8 @@ | |||
| ############################################################################## | |||
| # Common variables | |||
| ############################################################################## | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case. This violates Terraform conventions and breaks consistency.
| ############################################################################## | |
| variable "resource_group_id" { |
| | <a name="input_request_metrics_enabled"></a> [request\_metrics\_enabled](#input\_request\_metrics\_enabled) | If set to `true`, all Object Storage bucket request metrics are sent to Cloud Monitoring. | `bool` | `true` | no | | ||
| | <a name="input_resource_group_id"></a> [resource\_group\_id](#input\_resource\_group\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | | ||
| | <a name="input_resource_gRoup_id"></a> [resource\_gRoup\_id](#input\_resource\_gRoup\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | | ||
| | <a name="input_resource_keys"></a> [resource\_keys](#input\_resource\_keys) | The definition of the resource keys to generate. [Learn more](https://registry.terraform.io/providers/IBM-Cloud/ibm/latest/docs/resources/resource_key). | <pre>list(object({<br/> name = string<br/> key_name = optional(string, null)<br/> generate_hmac_credentials = optional(bool, false)<br/> role = optional(string, "Reader")<br/> service_id_crn = optional(string, null)<br/> }))</pre> | `[]` | no | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case. This violates Terraform conventions and breaks consistency.
| | <a name="input_resource_keys"></a> [resource\_keys](#input\_resource\_keys) | The definition of the resource keys to generate. [Learn more](https://registry.terraform.io/providers/IBM-Cloud/ibm/latest/docs/resources/resource_key). | <pre>list(object({<br/> name = string<br/> key_name = optional(string, null)<br/> generate_hmac_credentials = optional(bool, false)<br/> role = optional(string, "Reader")<br/> service_id_crn = optional(string, null)<br/> }))</pre> | `[]` | no | | |
| | <a name="input_resource_group_id"></a> [resource\\_group\\_id](#input\\_resource\\_group\\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | |
|
🤖 Reviewer - Status: ✖️ BLOCKING 2 issues found - see inline comments below for details and suggested fixes. Key actions needed:
Files reviewed: 2 | Lines changed: 4 |
terraform-ibm-modules-ops
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See summary comment above for review overview.
| | <a name="input_object_lock_duration_days"></a> [object\_lock\_duration\_days](#input\_object\_lock\_duration\_days) | The number of days for the object lock duration. If you specify a number of days, do not specify a value for `object_lock_duration_years`. Applies only if `create_cos_bucket` is set to `true`. | `number` | `0` | no | | ||
| | <a name="input_object_lock_duration_years"></a> [object\_lock\_duration\_years](#input\_object\_lock\_duration\_years) | The number of years for the object lock duration. If you specify a number of years, do not specify a value for `object_lock_duration_days`. Applies only if `create_cos_bucket` is set to `true`. | `number` | `0` | no | | ||
| | <a name="input_object_locking_enabled"></a> [object\_locking\_enabled](#input\_object\_locking\_enabled) | Whether to create an object lock configuration. If set to true, `object_versioning_enabled` and `create_cos_bucket` must also be set to `true`. | `bool` | `false` | no | | ||
| | <a name="input_object_versioning_enabled"></a> [object\_versioning\_enabled](#input\_object\_versioning\_enabled) | Whether to enable object versioning to keep multiple versions of an object in a bucket. Can't be used with retention rule. Applies only if `create_cos_bucket` is set to `true`. | `bool` | `false` | no | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case. This violates Terraform conventions and breaks consistency.
| | <a name="input_object_versioning_enabled"></a> [object\_versioning\_enabled](#input\_object\_versioning\_enabled) | Whether to enable object versioning to keep multiple versions of an object in a bucket. Can't be used with retention rule. Applies only if `create_cos_bucket` is set to `true`. | `bool` | `false` | no | | |
| | <a name="input_resource_group_id"></a> [resource_group_id](#input_resource_group_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | |
| @@ -1,8 +1,8 @@ | |||
| ############################################################################## | |||
| # Common variables | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case. This violates Terraform conventions and breaks consistency.
| # Common variables | |
| variable "resource_group_id" { |
|
🤖 Reviewer - Status: ❌ BLOCKING 2 issues found - see inline comments below for details and suggested fixes. Key actions needed:
Files reviewed: 2 | Lines changed: 4 |
terraform-ibm-modules-ops
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See summary comment above for review overview.
| | <a name="input_object_lock_duration_days"></a> [object\_lock\_duration\_days](#input\_object\_lock\_duration\_days) | The number of days for the object lock duration. If you specify a number of days, do not specify a value for `object_lock_duration_years`. Applies only if `create_cos_bucket` is set to `true`. | `number` | `0` | no | | ||
| | <a name="input_object_lock_duration_years"></a> [object\_lock\_duration\_years](#input\_object\_lock\_duration\_years) | The number of years for the object lock duration. If you specify a number of years, do not specify a value for `object_lock_duration_days`. Applies only if `create_cos_bucket` is set to `true`. | `number` | `0` | no | | ||
| | <a name="input_object_locking_enabled"></a> [object\_locking\_enabled](#input\_object\_locking\_enabled) | Whether to create an object lock configuration. If set to true, `object_versioning_enabled` and `create_cos_bucket` must also be set to `true`. | `bool` | `false` | no | | ||
| | <a name="input_object_versioning_enabled"></a> [object\_versioning\_enabled](#input\_object\_versioning\_enabled) | Whether to enable object versioning to keep multiple versions of an object in a bucket. Can't be used with retention rule. Applies only if `create_cos_bucket` is set to `true`. | `bool` | `false` | no | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case. This violates Terraform conventions and breaks consistency.
| | <a name="input_object_versioning_enabled"></a> [object\_versioning\_enabled](#input\_object\_versioning\_enabled) | Whether to enable object versioning to keep multiple versions of an object in a bucket. Can't be used with retention rule. Applies only if `create_cos_bucket` is set to `true`. | `bool` | `false` | no | | |
| | <a name="input_resource_group_id"></a> [resource_group_id](#input_resource_group_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | |
| @@ -1,8 +1,8 @@ | |||
| ############################################################################## | |||
| # Common variables | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case. This violates Terraform conventions and breaks consistency.
| # Common variables | |
| variable "resource_group_id" { |
|
🤖 Reviewer - Status: 2 issues found - see inline comments below for details and suggested fixes. Key actions needed:
Files reviewed: 2 | Lines changed: 4 |
terraform-ibm-modules-ops
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See summary comment above for review overview.
| | <a name="input_object_lock_duration_days"></a> [object\_lock\_duration\_days](#input\_object\_lock\_duration\_days) | The number of days for the object lock duration. If you specify a number of days, do not specify a value for `object_lock_duration_years`. Applies only if `create_cos_bucket` is set to `true`. | `number` | `0` | no | | ||
| | <a name="input_object_lock_duration_years"></a> [object\_lock\_duration\_years](#input\_object\_lock\_duration\_years) | The number of years for the object lock duration. If you specify a number of years, do not specify a value for `object_lock_duration_days`. Applies only if `create_cos_bucket` is set to `true`. | `number` | `0` | no | | ||
| | <a name="input_object_locking_enabled"></a> [object\_locking\_enabled](#input\_object\_locking\_enabled) | Whether to create an object lock configuration. If set to true, `object_versioning_enabled` and `create_cos_bucket` must also be set to `true`. | `bool` | `false` | no | | ||
| | <a name="input_object_versioning_enabled"></a> [object\_versioning\_enabled](#input\_object\_versioning\_enabled) | Whether to enable object versioning to keep multiple versions of an object in a bucket. Can't be used with retention rule. Applies only if `create_cos_bucket` is set to `true`. | `bool` | `false` | no | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case. This violates Terraform conventions and breaks consistency.
| | <a name="input_object_versioning_enabled"></a> [object\_versioning\_enabled](#input\_object\_versioning\_enabled) | Whether to enable object versioning to keep multiple versions of an object in a bucket. Can't be used with retention rule. Applies only if `create_cos_bucket` is set to `true`. | `bool` | `false` | no | | |
| | <a name="input_resource_group_id"></a> [resource_group_id](#input_resource_group_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | |
| @@ -1,8 +1,8 @@ | |||
| ############################################################################## | |||
| # Common variables | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case. This violates Terraform conventions and breaks consistency.
| # Common variables | |
| variable "resource_group_id" { |
|
🤖 Reviewer - Status: 2 issues found - see inline comments below for details and suggested fixes. Key actions needed:
Files reviewed: 2 | Lines changed: 4 |
|
🤖 Reviewer - This commit has already been reviewed. Please see previous comments for feedback. |
|
Review in progress... |
terraform-ibm-modules-ops
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See summary comment above for review overview.
| | <a name="input_object_lock_duration_days"></a> [object\_lock\_duration\_days](#input\_object\_lock\_duration\_days) | The number of days for the object lock duration. If you specify a number of days, do not specify a value for `object_lock_duration_years`. Applies only if `create_cos_bucket` is set to `true`. | `number` | `0` | no | | ||
| | <a name="input_object_lock_duration_years"></a> [object\_lock\_duration\_years](#input\_object\_lock\_duration\_years) | The number of years for the object lock duration. If you specify a number of years, do not specify a value for `object_lock_duration_days`. Applies only if `create_cos_bucket` is set to `true`. | `number` | `0` | no | | ||
| | <a name="input_object_locking_enabled"></a> [object\_locking\_enabled](#input\_object\_locking\_enabled) | Whether to create an object lock configuration. If set to true, `object_versioning_enabled` and `create_cos_bucket` must also be set to `true`. | `bool` | `false` | no | | ||
| | <a name="input_object_versioning_enabled"></a> [object\_versioning\_enabled](#input\_object\_versioning\_enabled) | Whether to enable object versioning to keep multiple versions of an object in a bucket. Can't be used with retention rule. Applies only if `create_cos_bucket` is set to `true`. | `bool` | `false` | no | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case. This violates Terraform conventions and breaks consistency.
| | <a name="input_object_versioning_enabled"></a> [object\_versioning\_enabled](#input\_object\_versioning\_enabled) | Whether to enable object versioning to keep multiple versions of an object in a bucket. Can't be used with retention rule. Applies only if `create_cos_bucket` is set to `true`. | `bool` | `false` | no | | |
| | <a name="input_resource_group_id"></a> [resource_group_id](#input_resource_group_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | |
| @@ -1,8 +1,8 @@ | |||
| ############################################################################## | |||
| # Common variables | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case. This violates Terraform conventions and breaks consistency.
| # Common variables | |
| variable "resource_group_id" { |
|
🤖 Reviewer - This commit has already been reviewed. Please see previous review comments for feedback. If you've pushed new changes, the new commit will be reviewed automatically. |
terraform-ibm-modules-ops
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review the changes made in this pull request.
|
See summary comment above for review overview. |
terraform-ibm-modules-ops
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See summary comment above for review overview.
| @@ -1,8 +1,8 @@ | |||
| ############################################################################## | |||
| # Common variables | |||
| ############################################################################## | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case. This violates Terraform conventions and breaks consistency.
| ############################################################################## | |
| variable "resource_group_id" { |
| | <a name="input_region"></a> [region](#input\_region) | The region to provision the bucket. If specified, set `cross_region_location` and `single_site_location` to `null`. | `string` | `"us-south"` | no | | ||
| | <a name="input_request_metrics_enabled"></a> [request\_metrics\_enabled](#input\_request\_metrics\_enabled) | If set to `true`, all Object Storage bucket request metrics are sent to Cloud Monitoring. | `bool` | `true` | no | | ||
| | <a name="input_resource_group_id"></a> [resource\_group\_id](#input\_resource\_group\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | | ||
| | <a name="input_resource_gRoup_id"></a> [resource\_gRoup\_id](#input\_resource\_gRoup\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case in documentation. This violates Terraform conventions and breaks consistency.
| | <a name="input_resource_gRoup_id"></a> [resource\_gRoup\_id](#input\_resource\_gRoup\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | | |
| | <a name="input_resource_group_id"></a> [resource\\_group\\_id](#input\\_resource\\_group\\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | |
|
🤖 Reviewer - Review in progress... |
terraform-ibm-modules-ops
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automated code review completed. See inline comments for details.
| ############################################################################## | ||
|
|
||
| variable "resource_group_id" { | ||
| variable "resource_gRoup_id" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Naming - Blocking] Variable uses mixed case (gRoup) instead of snake_case. This violates Terraform naming conventions and breaks consistency with the rest of the codebase.
| variable "resource_gRoup_id" { | |
| variable "resource_group_id" { |
| | <a name="input_region"></a> [region](#input\_region) | The region to provision the bucket. If specified, set `cross_region_location` and `single_site_location` to `null`. | `string` | `"us-south"` | no | | ||
| | <a name="input_request_metrics_enabled"></a> [request\_metrics\_enabled](#input\_request\_metrics\_enabled) | If set to `true`, all Object Storage bucket request metrics are sent to Cloud Monitoring. | `bool` | `true` | no | | ||
| | <a name="input_resource_group_id"></a> [resource\_group\_id](#input\_resource\_group\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | | ||
| | <a name="input_resource_gRoup_id"></a> [resource\_gRoup\_id](#input\_resource\_gRoup\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Documentation - Blocking] Documentation updated to match incorrect variable name. Variable name must use snake_case per Terraform conventions.
| | <a name="input_resource_gRoup_id"></a> [resource\_gRoup\_id](#input\_resource\_gRoup\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | | |
| | <a name="input_resource_group_id"></a> [resource\_group\_id](#input\_resource\_group\_id) | The resource group ID for the Object Storage instance. Required if `create_cos_instance` is set to `true`. | `string` | `null` | no | |
|
/bob review |
3 similar comments
|
/bob review |
|
/bob review |
|
/bob review |
|
🤖 Bob Reviewer - This commit has already been reviewed. Please see previous comments for feedback. |
Description
Release required?
x.x.X)x.X.x)X.x.x)Release notes content
Run the pipeline
If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.
Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:
Checklist for reviewers
For mergers