feat: upgrade from bitnami to official metrics server charts #15
Conversation
WalkthroughThis pull request updates documentation and configuration for a Terraform module. The README now uses Markdown line breaks instead of HTML breaks in variable descriptions. In the Terraform configuration files, changes include removing an obsolete local variable, updating namespace sourcing and label merging for the metrics server module, and modifying default values and descriptions for several variables, along with introducing a new variable for the metrics server component. Changes
Sequence Diagram(s)sequenceDiagram
participant Var as variables.tf
participant Main as main.tf
participant MS as Metrics Server Module
Var->>Main: Provide updated variable defaults & new metrics_server_component
Main->>MS: Forward updated config (namespace, labels, podLabels)
Note over Main,MS: Metrics Server module processes new settings
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
RoseSecurity
added
major
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Require terratestWonderful, this rule succeeded.This rule require terratest status
|
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main.tf (1)
1-4: State Migration Validation for Namespace Resource
Themovedblock correctly migrates the namespace resource fromkubernetes_namespace.defaulttomodule.metrics_server.kubernetes_namespace.default. Please verify that the state migration has been executed safely in existing environments.README.md (1)
118-121: Minor Grammar and Style Suggestions
A couple of minor grammatical improvements are recommended:
- In the description for
id_length_limit, consider changing “Set tonullfor keep the existing setting…” to “Set tonullto keep the existing setting…”- In the descriptions for
nameandnamespaceinputs, adding a comma after “Usually” (e.g., “Usually, the component or solution name…” and “Usually, an abbreviation…”) can enhance clarity.
These are small nitpicks that could improve the overall quality of the documentation.Also applies to: 138-139
🧰 Tools
🪛 LanguageTool
[grammar] ~118-~118: Did you mean “too false to”?
Context: ..."> enabled | Set to false to prevent the module from creating any re...(TOO_ADJECTIVE_TO)
[grammar] ~120-~120: This verb might not be in the correct form.
Context: ...se so the full diff of what is changing can been seen in the plan |bool|false| n...(MD_BE)
[grammar] ~121-~121: The verb ‘keep’ seems to be in the wrong form here.
Context: ... for unlimited length.
Set tonullfor keep the existing setting, which defaults to...(FOR_VB)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)src/main.tf(3 hunks)src/variables.tf(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~118-~118: Did you mean “too false to”?
Context: ..."> enabled | Set to false to prevent the module from creating any re...
(TOO_ADJECTIVE_TO)
[grammar] ~120-~120: This verb might not be in the correct form.
Context: ...se so the full diff of what is changing can been seen in the plan | bool | false | n...
(MD_BE)
[grammar] ~121-~121: The verb ‘keep’ seems to be in the wrong form here.
Context: ... for unlimited length.
Set to null for keep the existing setting, which defaults to...
(FOR_VB)
[typographical] ~138-~138: Consider adding a comma after ‘Usually’ for more clarity.
Context: ... name | ID element. Usually the component or solution name, e.g. 'a...
(RB_LY_COMMA)
[typographical] ~139-~139: Consider adding a comma after ‘Usually’ for more clarity.
Context: ...space](#input_namespace) | ID element. Usually an abbreviation of your organization na...
(RB_LY_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: component / terraform / Lint (./src)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (8)
src/main.tf (2)
23-24: Introduce Configurable Namespace Labels
The addition ofkubernetes_namespace_labels = merge(module.this.tags, { name = var.kubernetes_namespace })enhances flexibility by allowing pod label customization. Confirm that merging tags in this manner does not unintentionally override any critical labels.
44-46: Update Pod Labels for Metrics Server
The modifications in thepodLabelsblock—updating therepovalue to"kubernetes-sigs-github-io"and settingcomponentto use the new variablevar.metrics_server_component—align the configuration with the official metrics server charts. Ensure that the documentation and any downstream dependencies reflect these label changes.src/variables.tf (4)
18-22: Update Chart Repository URL
The default forchart_repositoryhas been updated from Bitnami to the official metrics server repository URL. Verify that the URL"https://kubernetes-sigs.github.io/metrics-server/"is correct and that any automated tooling (e.g., CI/CD pipelines) is updated to reflect this change.
24-28: Upgrade Chart Version
The default value forchart_versionhas been changed from"6.2.6"to"3.11.0". Please ensure this version is compatible with your deployment and that any breaking changes in the new chart version have been addressed.
102-107: Correct EKS Component Description
The description foreks_component_namehas been updated to"The name of the EKS component", which improves readability and professional consistency.
108-113: Introduce Metrics Server Component Variable
A new variablemetrics_server_componenthas been added with a default value of"eks-metrics-server". This allows the component name to be configurable and avoids hardcoding. Ensure that references to this variable in the module (e.g., inpodLabels) and in the documentation are updated accordingly.README.md (2)
3-7: Documentation Banner and Badge Section
No functionality change; the banner and badges remain intact. The removal of HTML<br/>tags in favor of Markdown line breaks elsewhere (as seen later) improves consistency.
104-148: Refine Input Variable Descriptions Formatting
The input variable descriptions have been updated to replace HTML<br/>tags with Markdown line breaks. This update improves the readability and consistency of the documentation for variables such asadditional_tag_map,attributes,delimiter, and others. Please review the rendered Markdown to ensure that the format meets your documentation standards.🧰 Tools
🪛 LanguageTool
[grammar] ~118-~118: Did you mean “too false to”?
Context: ..."> enabled | Set to false to prevent the module from creating any re...(TOO_ADJECTIVE_TO)
[grammar] ~120-~120: This verb might not be in the correct form.
Context: ...se so the full diff of what is changing can been seen in the plan |bool|false| n...(MD_BE)
[grammar] ~121-~121: The verb ‘keep’ seems to be in the wrong form here.
Context: ... for unlimited length.
Set tonullfor keep the existing setting, which defaults to...(FOR_VB)
[typographical] ~138-~138: Consider adding a comma after ‘Usually’ for more clarity.
Context: ... name | ID element. Usually the component or solution name, e.g. 'a...(RB_LY_COMMA)
[typographical] ~139-~139: Consider adding a comma after ‘Usually’ for more clarity.
Context: ...space](#input_namespace) | ID element. Usually an abbreviation of your organization na...(RB_LY_COMMA)
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
/terratest |
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
/terratest |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
README.md (7)
104-104: Formatting Consistency & Punctuation:
The description for theadditional_tag_mapinput now uses Markdown<br>instead of<br/>, which improves consistency. However, static analysis suggests that a period may be missing at the end of the first sentence. Please review if adding terminal punctuation would improve clarity.🧰 Tools
🪛 LanguageTool
[uncategorized] ~104-~104: A period might be missing here.
Context: ...s_list_of_maps. Not added totagsorid`.
This is for some rare cases wher...(AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
106-106: Punctuation Suggestion for Clarity:
In theattributesinput description, consider adding a comma after the parenthetical examples (e.g., after “cluster”) to improve readability.🧰 Tools
🪛 LanguageTool
[uncategorized] ~106-~106: A comma might be missing here.
Context: ...(e.g.workersorcluster) to add toid,
in the order they appear in the ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
107-107: Article Inclusion Recommendation:
The description for thechartinput would be clearer by inserting the indefinite article “a” before “local path” (i.e. “a local path”).🧰 Tools
🪛 LanguageTool
[uncategorized] ~107-~107: You might be missing the article “a” here.
Context: ... to be installed. The chart name can be local path, a URL to a chart, or the name of ...(AI_EN_LECTOR_MISSING_DETERMINER_A)
121-121: Grammar Improvement Needed:
In theid_length_limitdescription, the phrase “for keep the existing setting” should be revised to “to keep the existing setting” for proper grammatical structure.🧰 Tools
🪛 LanguageTool
[grammar] ~121-~121: The verb ‘keep’ seems to be in the wrong form here.
Context: ... for unlimited length.
Set tonullfor keep the existing setting, which defaults to...(FOR_VB)
138-138: Phrasing & Punctuation Enhancement:
The description for thenameinput would benefit from a comma after “Usually” and a slight rephrasing. Consider revising to: “ID element. Usually, the component or solution name (e.g., ‘app’ or ‘jenkins’). This is the only ID element that is not also included as atag– the “name” tag is set to the fullidstring.”🧰 Tools
🪛 LanguageTool
[typographical] ~138-~138: Consider adding a comma after ‘Usually’ for more clarity.
Context: ... name | ID element. Usually the component or solution name, e.g. 'a...(RB_LY_COMMA)
[uncategorized] ~138-~138: Possible missing preposition found.
Context: ...ID element not also included as atag.
The "name" tag is set to the fullids...(AI_EN_LECTOR_MISSING_PREPOSITION)
139-139: Clarity Improvement:
In thenamespaceinput description, adding a comma after “Usually” would help clarify the text.🧰 Tools
🪛 LanguageTool
[typographical] ~139-~139: Consider adding a comma after ‘Usually’ for more clarity.
Context: ...space](#input_namespace) | ID element. Usually an abbreviation of your organization na...(RB_LY_COMMA)
100-152: New Variable Documentation Reminder:
The PR objectives mention the introduction of a new variable for the metrics server component (i.e.metrics_server_component). However, it isn’t documented in this README. Please update the Inputs section to include details on this new variable so that users understand its purpose and usage.🧰 Tools
🪛 LanguageTool
[uncategorized] ~104-~104: A period might be missing here.
Context: ...s_list_of_maps. Not added totagsorid`.
This is for some rare cases wher...(AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
[uncategorized] ~106-~106: A comma might be missing here.
Context: ...(e.g.workersorcluster) to add toid,
in the order they appear in the ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~107-~107: You might be missing the article “a” here.
Context: ... to be installed. The chart name can be local path, a URL to a chart, or the name of ...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[grammar] ~118-~118: Did you mean “too false to”?
Context: ..."> enabled | Set to false to prevent the module from creating any re...(TOO_ADJECTIVE_TO)
[grammar] ~120-~120: This verb might not be in the correct form.
Context: ...se so the full diff of what is changing can been seen in the plan |bool|false| n...(MD_BE)
[grammar] ~121-~121: The verb ‘keep’ seems to be in the wrong form here.
Context: ... for unlimited length.
Set tonullfor keep the existing setting, which defaults to...(FOR_VB)
[typographical] ~138-~138: Consider adding a comma after ‘Usually’ for more clarity.
Context: ... name | ID element. Usually the component or solution name, e.g. 'a...(RB_LY_COMMA)
[uncategorized] ~138-~138: Possible missing preposition found.
Context: ...ID element not also included as atag.
The "name" tag is set to the fullids...(AI_EN_LECTOR_MISSING_PREPOSITION)
[typographical] ~139-~139: Consider adding a comma after ‘Usually’ for more clarity.
Context: ...space](#input_namespace) | ID element. Usually an abbreviation of your organization na...(RB_LY_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~104-~104: A period might be missing here.
Context: ...s_list_of_maps. Not added to tagsorid`.
This is for some rare cases wher...
(AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
[uncategorized] ~106-~106: A comma might be missing here.
Context: ...(e.g. workers or cluster) to add to id,
in the order they appear in the ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~107-~107: You might be missing the article “a” here.
Context: ... to be installed. The chart name can be local path, a URL to a chart, or the name of ...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[grammar] ~118-~118: Did you mean “too false to”?
Context: ..."> enabled | Set to false to prevent the module from creating any re...
(TOO_ADJECTIVE_TO)
[grammar] ~120-~120: This verb might not be in the correct form.
Context: ...se so the full diff of what is changing can been seen in the plan | bool | false | n...
(MD_BE)
[grammar] ~121-~121: The verb ‘keep’ seems to be in the wrong form here.
Context: ... for unlimited length.
Set to null for keep the existing setting, which defaults to...
(FOR_VB)
[typographical] ~138-~138: Consider adding a comma after ‘Usually’ for more clarity.
Context: ... name | ID element. Usually the component or solution name, e.g. 'a...
(RB_LY_COMMA)
[uncategorized] ~138-~138: Possible missing preposition found.
Context: ...ID element not also included as a tag.
The "name" tag is set to the full id s...
(AI_EN_LECTOR_MISSING_PREPOSITION)
[typographical] ~139-~139: Consider adding a comma after ‘Usually’ for more clarity.
Context: ...space](#input_namespace) | ID element. Usually an abbreviation of your organization na...
(RB_LY_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (1)
README.md (1)
109-109: Verify Chart Repository URL:
According to the PR objective of upgrading from Bitnami to official metrics server charts, please confirm whether the default value forchart_repositoryshould be updated from"https://charts.bitnami.com/bitnami"to"https://kubernetes-sigs.github.io/metrics-server/". If so, update the README accordingly.
|
/terratest |
1 similar comment
|
/terratest |
|
These changes were released in v2.0.0. |
what and why
https://kubernetes-sigs.github.io/metrics-server/testing
references
Summary by CodeRabbit
Documentation
Chores
New Features
Tests