Fix issues related to empty stack dir, optional cluster_name_pattern and better of use_eks flag #734
Conversation
…and better of use_eks flag Signed-off-by: Sahadat Hossain <shahincsejnu@gmail.com>
📝 WalkthroughWalkthroughThis pull request introduces several changes across multiple files. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range comments (3)
website/docs/core-concepts/vendor/vendor.mdx (3)
Line range hint
17-27: LGTM: Comprehensive use cases with a suggestionThe expanded Use-cases section greatly enhances the documentation by providing detailed, practical examples of vendoring applications. This aligns well with the PR objective of improving documentation clarity.
Consider adding a brief introductory sentence before listing the use cases to provide context, such as:
"Vendoring in Atmos can be applied in various scenarios to improve dependency management and code reuse. Here are some key use cases:"
This addition would help readers transition into the detailed list of use cases more smoothly.
🧰 Tools
🪛 LanguageTool
[style] ~66-~66: This phrase is redundant. Consider using “inside”.
Context: ...t) Usingcomponent.yamlmanifest file inside of a component directory. See below. The ...(OUTSIDE_OF)
Line range hint
41-50: Suggestion: Merge duplicate Use-cases sectionsThere are currently two separate Use-cases sections in the document. To improve readability and avoid confusion, consider merging this section with the earlier Use-cases section (lines 17-27). The content in both sections is valuable, but combining them would create a more cohesive and comprehensive list of use cases for Atmos vendoring.
Here's a suggested approach:
- Move the unique use cases from this section to the earlier Use-cases section.
- Combine overlapping points to avoid redundancy.
- Remove this duplicate section entirely.
This reorganization will enhance the overall structure and flow of the document.
🧰 Tools
🪛 LanguageTool
[style] ~66-~66: This phrase is redundant. Consider using “inside”.
Context: ...t) Usingcomponent.yamlmanifest file inside of a component directory. See below. The ...(OUTSIDE_OF)
Line range hint
52-66: LGTM: Updated link with a minor suggestionThe update to the Component Manifest link is correct and aligns with the PR objective of fixing website URL links. This change will ensure users are directed to the appropriate documentation.
There's a minor grammatical issue in the last sentence of this section. Consider revising it as follows:
- A `component.yaml` file placed into a component's directory is used to describe the vendoring config for one component only. + A `component.yaml` file placed in a component's directory is used to describe the vendoring configuration for that specific component only.This revision improves clarity and grammatical correctness.
🧰 Tools
🪛 LanguageTool
[style] ~66-~66: This phrase is redundant. Consider using “inside”.
Context: ...t) Usingcomponent.yamlmanifest file inside of a component directory. See below. The ...(OUTSIDE_OF)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
- .gitignore (1 hunks)
- go.mod (5 hunks)
- internal/exec/helmfile.go (1 hunks)
- internal/exec/validate_stacks.go (2 hunks)
- pkg/config/config.go (0 hunks)
- pkg/config/utils.go (2 hunks)
- website/docs/core-concepts/vendor/vendor.mdx (1 hunks)
💤 Files with no reviewable changes (1)
- pkg/config/config.go
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/vendor/vendor.mdx
[style] ~66-~66: This phrase is redundant. Consider using “inside”.
Context: ...t) Usingcomponent.yamlmanifest file inside of a component directory. See below. The ...(OUTSIDE_OF)
🔇 Additional comments (11)
.gitignore (1)
37-37: LGTM: Addition ofvendorto .gitignore is appropriate.The addition of
vendorto the .gitignore file is a good practice for Go projects using vendoring for dependency management. This change ensures that vendored dependencies are not tracked by Git, which is typically desired as these files are usually managed by the Go toolchain.This addition aligns well with the PR objectives related to improving dependency management. It suggests that the project may be adopting or refining its approach to vendoring, which can offer benefits such as reproducible builds and easier offline development.
website/docs/core-concepts/vendor/vendor.mdx (2)
Line range hint
1-15: LGTM: Enhanced introduction and overviewThe changes in the introduction and first paragraph effectively clarify the purpose and benefits of vendoring in Atmos. The content accurately describes how vendoring can help manage dependencies, particularly for Terraform, and outlines its advantages for standardization, reuse, and compliance.
🧰 Tools
🪛 LanguageTool
[style] ~66-~66: This phrase is redundant. Consider using “inside”.
Context: ...t) Usingcomponent.yamlmanifest file inside of a component directory. See below. The ...(OUTSIDE_OF)
Line range hint
29-39: LGTM: Detailed feature listThe updated Features section provides a comprehensive list of sources from which Atmos can copy components and artifacts. This addition enhances the documentation by clearly outlining the capabilities of Atmos vendoring. The mention of inspiration from VMware Tanzu's
vendirtool provides useful context for users familiar with other tools in the ecosystem.🧰 Tools
🪛 LanguageTool
[style] ~66-~66: This phrase is redundant. Consider using “inside”.
Context: ...t) Usingcomponent.yamlmanifest file inside of a component directory. See below. The ...(OUTSIDE_OF)
internal/exec/helmfile.go (1)
Line range hint
1-338: Summary of review for internal/exec/helmfile.goThe changes in this file improve error handling for EKS-related operations by adding a check for the AWS region. This aligns with the PR objectives of enhancing the functionality of atmos helmfile commands, particularly in the context of EKS usage.
However, there's an opportunity to further improve the code structure to better support optional EKS usage, which would more fully align with the PR objective of making atmos helmfile commands not require EKS.
Overall, the changes are a step in the right direction, but further refactoring could enhance the flexibility and maintainability of the code.
go.mod (7)
22-22: Verify the version ofgh.neting.cc/hashicorp/hclThe dependency
github.com/hashicorp/hclhas been updated to versionv1.0.1-vault-5. This appears to be a custom or forked version specific to Vault. Please confirm that this is the intended version and that it is compatible with your codebase.
48-93: Ensure compatibility of updated indirect dependenciesA large number of indirect dependencies have been added or updated, including AWS SDKs, Google Cloud libraries, and OpenTelemetry packages. Notable updates include:
github.com/aws/aws-sdk-gotov1.55.5gh.neting.cc/aws/aws-sdk-go-v2tov1.32.2cloud.google.com/gotov0.116.0- OpenTelemetry packages to
v1.31.0Please ensure that these updated dependencies are necessary, compatible with your codebase, and do not introduce any breaking changes or security vulnerabilities.
122-123: Confirm necessity of Envoy Proxy dependenciesThe dependencies
github.com/envoyproxy/go-control-planeandgh.neting.cc/envoyproxy/protoc-gen-validatehave been added as indirect dependencies. Ensure these are required for your project and verify their compatibility.
152-155: Verify updates to HashiCorp dependenciesSeveral HashiCorp dependencies, including
github.com/hashicorp/go-hclog,github.com/hashicorp/go-plugin, andgh.neting.cc/hashicorp/terraform-json, have been updated. Please confirm that these updates align with your project's requirements and that they do not introduce any breaking changes.
168-170: Review updates to Vault API and SDKThe dependencies
github.com/hashicorp/vault/apiandgh.neting.cc/hashicorp/vault/sdkhave been updated to newer versions. Ensure that your code is compatible with these versions and that any necessary code changes are made.
209-209: Check compatibility ofgh.neting.cc/rs/zerologupdateThe
github.com/rs/zerologdependency has been updated tov1.33.0. Verify that this update does not introduce any breaking changes and that all logging functionalities work as expected.
271-272: Ensure compatibility with updated Kubernetes packagesThe Kubernetes client libraries
k8s.io/client-go v0.31.1andk8s.io/utilshave been updated. Check that these versions are compatible with your codebase and update any Kubernetes-related code if necessary.
| dir, err := os.ReadDir(cliConfig.StacksBaseAbsolutePath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(dir) == 0 { | ||
| validationErrorMessages = append(validationErrorMessages, "The 'stacks' directory is empty. Please create stack manifest files in the 'stacks' directory before running the 'validate stacks' command.") | ||
| return errors.New(strings.Join(validationErrorMessages, "\n\n")) | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Approve changes with a minor suggestion for consistency
The new check for an empty stacks directory is a valuable addition. It enhances the validation process by ensuring the basic structure is in place before proceeding with more detailed checks. This aligns well with the PR objectives of improving error handling and user experience.
For consistency with the rest of the function, consider moving the error return to the end of the function:
if len(dir) == 0 {
validationErrorMessages = append(validationErrorMessages, "The 'stacks' directory is empty. Please create stack manifest files in the 'stacks' directory before running the 'validate stacks' command.")
- return errors.New(strings.Join(validationErrorMessages, "\n\n"))
}
// ... rest of the function ...
if len(validationErrorMessages) > 0 {
return errors.New(strings.Join(validationErrorMessages, "\n\n"))
}This change would allow for collecting all validation errors, including the empty directory check, before returning.
📝 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.
| dir, err := os.ReadDir(cliConfig.StacksBaseAbsolutePath) | |
| if err != nil { | |
| return err | |
| } | |
| if len(dir) == 0 { | |
| validationErrorMessages = append(validationErrorMessages, "The 'stacks' directory is empty. Please create stack manifest files in the 'stacks' directory before running the 'validate stacks' command.") | |
| return errors.New(strings.Join(validationErrorMessages, "\n\n")) | |
| } | |
| dir, err := os.ReadDir(cliConfig.StacksBaseAbsolutePath) | |
| if err != nil { | |
| return err | |
| } | |
| if len(dir) == 0 { | |
| validationErrorMessages = append(validationErrorMessages, "The 'stacks' directory is empty. Please create stack manifest files in the 'stacks' directory before running the 'validate stacks' command.") | |
| } | |
| // ... rest of the function ... | |
| if len(validationErrorMessages) > 0 { | |
| return errors.New(strings.Join(validationErrorMessages, "\n\n")) | |
| } |
| github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect | ||
| github.com/beorn7/perks v1.0.1 // indirect | ||
| github.com/bgentry/go-netrc v0.0.0-20140422174119-9fd32a8b3d3d // indirect | ||
| github.com/bytecodealliance/wasmtime-go/v3 v3.0.2 // indirect | ||
| github.com/cenkalti/backoff/v3 v3.2.2 // indirect | ||
| github.com/cenkalti/backoff/v4 v4.3.0 // indirect |
There was a problem hiding this comment.
Remove duplicate versions of github.com/cenkalti/backoff
Both github.com/cenkalti/backoff/v3 and github.com/cenkalti/backoff/v4 are listed as indirect dependencies. Having multiple major versions can lead to conflicts and increase binary size. Confirm which version is required and remove the unnecessary one.
Apply this diff to remove the older version if not needed:
- github.com/cenkalti/backoff/v3 v3.2.2 // indirectCommittable suggestion was skipped due to low confidence.
| @@ -103,9 +103,6 @@ func InitCliConfig(configAndStacksInfo schema.ConfigAndStacksInfo, processStacks | |||
| v.SetConfigType("yaml") | |||
| v.SetTypeByDefaultValue(true) | |||
|
|
|||
| // Default configuration values | |||
| v.SetDefault("components.helmfile.use_eks", true) | |||
There was a problem hiding this comment.
I think this is the fix, by not defaulting it. This parameter was never documented by us. Please update the available parameters for helmfile in the helmfile docs.
| @@ -275,11 +278,6 @@ func checkHelmfileConfig(cliConfig schema.CliConfiguration) error { | |||
| return errors.New("Helm AWS profile pattern must be provided in 'components.helmfile.helm_aws_profile_pattern' config or " + | |||
| "'ATMOS_COMPONENTS_HELMFILE_HELM_AWS_PROFILE_PATTERN' ENV variable") | |||
| } | |||
|
|
|||
| if len(cliConfig.Components.Helmfile.ClusterNamePattern) < 1 { | |||
| return errors.New("Cluster name pattern must be provided in 'components.helmfile.cluster_name_pattern' config or " + | |||
There was a problem hiding this comment.
@aknysh do we still require this if use_eks is true?
There was a problem hiding this comment.
ClusterNamePattern is needed to access the EKS cluster
There was a problem hiding this comment.
@shahincsejnu we should require cluster_name_pattern if use_eks is true
There was a problem hiding this comment.
@osterman okay, made it required!
|
@shahincsejnu thanks, please address the comments and resolve the conflicts |
aknysh
added
the
no-release
Signed-off-by: Sahadat Hossain <shahincsejnu@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range comments (1)
internal/exec/helmfile.go (1)
Line range hint
32-285: Consider broader refactoring for optional EKS usageWhile the current changes improve error handling, there's an opportunity to further enhance the
ExecuteHelmfilefunction to make EKS usage truly optional, as suggested in previous review comments.Consider refactoring the EKS-specific operations into a separate function that's only called when
cliConfig.Components.Helmfile.UseEKSis true. This would make the mainExecuteHelmfilefunction more agnostic to whether EKS is being used or not, improving flexibility for non-EKS use cases.Here's a high-level suggestion for the refactoring:
- Create a new function
setupEKSthat encapsulates all EKS-specific logic.- In
ExecuteHelmfile, callsetupEKSonly whencliConfig.Components.Helmfile.UseEKSis true.- Move all EKS-specific checks (like the new region check) into
setupEKS.This refactoring would align better with the PR objective of making atmos helmfile commands not require EKS and improve the overall structure of the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
- go.mod (4 hunks)
- internal/exec/helmfile.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (8)
internal/exec/helmfile.go (1)
283-283: Approve the error message improvementThe modification to consolidate the error message for the missing cluster name pattern into a single line improves readability and consistency.
go.mod (7)
22-22: Verify the use of custom versionv1.0.1-vault-5forgh.neting.cc/hashicorp/hcl.The dependency
github.com/hashicorp/hclis set to versionv1.0.1-vault-5, which appears to be a custom or forked version specific to HashiCorp Vault. Ensure that this version is intentionally used and that it doesn't introduce compatibility issues with other components relying onhcl.
73-91: Ensure compatibility with updated AWS SDK dependencies.The AWS SDK dependencies have been significantly upgraded:
github.com/aws/aws-sdk-goupdated tov1.55.5gh.neting.cc/aws/aws-sdk-go-v2updated tov1.32.2- Multiple related AWS modules have also been updated.
Verify that your codebase is compatible with these new versions, as there may be breaking changes or deprecations that require code adjustments.
128-128: Confirm compatibility withgh.neting.cc/go-jose/go-jose/v4upgrade.The library
github.com/go-jose/go-josehas been updated to major versionv4.0.4. Major version changes can introduce breaking changes. Ensure that any code using this library is updated accordingly.
169-169: Review update ofgh.neting.cc/klauspost/compresstov1.17.11.The compression library has been updated. While minor version updates are generally safe, it's prudent to test compression and decompression functionality to ensure there are no regressions.
189-191: Check the use of pseudo-version forgh.neting.cc/planetscale/vtprotobuf.The dependency is specified with a pseudo-version:
v0.6.1-0.20240319094008-0393e58bdf10Pseudo-versions are generally used for versions that are not tagged. Confirm that this is the intended version and consider using a tagged release if available for better stability.
227-231: Verify updates to OpenTelemetry packages.The OpenTelemetry dependencies have been updated to
v1.31.0:
go.opentelemetry.io/otelgo.opentelemetry.io/otel/metricgo.opentelemetry.io/otel/sdk- And others.
Ensure that your tracing and metrics implementation is compatible with these updated packages, as there may be API changes.
259-259: Ensure compatibility withk8s.io/client-go v0.31.1.The Kubernetes client-go library has been updated to
v0.31.1. Major version updates can include breaking changes. Verify that any Kubernetes interactions in your codebase are updated to work with this version.
| if context.Region == "" { | ||
| return errors.New("aws region must be specified") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for AWS region check
The addition of an AWS region check is a good improvement. However, to fully align with the PR objectives and previous review comments, consider the following enhancements:
- Make the check conditional on EKS usage.
- Improve the error message to be more specific.
Here's a suggested implementation:
-if context.Region == "" {
- return errors.New("aws region must be specified")
-}
+if cliConfig.Components.Helmfile.UseEKS {
+ if context.Region == "" {
+ return errors.New("AWS region must be specified when using EKS")
+ }
+}This change makes the region check specific to EKS usage and provides a more informative error message.
📝 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.
| if context.Region == "" { | |
| return errors.New("aws region must be specified") | |
| } | |
| if cliConfig.Components.Helmfile.UseEKS { | |
| if context.Region == "" { | |
| return errors.New("AWS region must be specified when using EKS") | |
| } | |
| } |
| cel.dev/expr v0.16.2 // indirect | ||
| cloud.google.com/go v0.116.0 // indirect | ||
| cloud.google.com/go/auth v0.9.8 // indirect | ||
| cloud.google.com/go/auth/oauth2adapt v0.2.4 // indirect | ||
| cloud.google.com/go/compute/metadata v0.5.2 // indirect | ||
| cloud.google.com/go/iam v1.2.1 // indirect | ||
| cloud.google.com/go/monitoring v1.21.1 // indirect | ||
| cloud.google.com/go/storage v1.44.0 // indirect |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Consider cleaning up unnecessary indirect dependencies.
Several new indirect dependencies have been added:
cel.dev/expr v0.16.2cloud.google.com/go v0.116.0cloud.google.com/go/auth v0.9.8- And others.
Indirect dependencies are usually managed automatically and might not need to be listed explicitly. Running go mod tidy can help remove unnecessary indirect dependencies and keep the go.mod file clean.
| go.uber.org/multierr v1.11.0 // indirect | ||
| go4.org/intern v0.0.0-20230525184215-6c62f75575cb // indirect | ||
| go4.org/unsafe/assume-no-moving-gc v0.0.0-20231121144256-b99613f794b6 // indirect | ||
| gocloud.dev v0.40.0 // indirect |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Assess the necessity of adding gocloud.dev v0.40.0.
The dependency gocloud.dev has been added. If your project doesn't directly use this package, and it's not required by your direct dependencies, consider removing it to minimize unnecessary dependencies.
@aknysh resolved & updated, pls have a look again, thanks! |
|
There are new merge conflicts. Also, please show screenshots validating the functionality against k3s. |
what
why
references
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
.gitignoreto streamline version control.