Add support for remote validation schemas #731
Conversation
|
Warning Rate limit exceeded@aknysh has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- examples/demo-context/atmos.yaml (1 hunks)
- internal/exec/validate_stacks.go (2 hunks)
- pkg/utils/file_utils.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
examples/demo-context/atmos.yaml (1)
5-5: Approved: Remote schema support implemented as intended.This change aligns well with the PR objectives by enabling remote schema support, which should help reduce redundancy in manifest file management. Great job!
A few suggestions to ensure robustness and usability:
- Please verify that the URL is accessible and returns the expected schema.
- Consider implementing a fallback mechanism (e.g., local cache) in case the remote URL is temporarily unavailable.
- Update the user documentation to reflect this new capability and provide guidance on using remote schemas securely.
To verify the URL accessibility and content, you can run the following script:
✅ Verification successful
Verified: Remote schema URL is accessible and contains valid JSON.
The remote schema URL
https://atmos.tools/schemas/atmos/atmos-manifest/1.0/atmos-manifest.jsonis accessible and returns valid JSON as expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the accessibility and content of the remote schema URL # Test: Check if the URL is accessible and returns a valid JSON schema curl -sSf "https://atmos.tools/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json" | jq empty # If the above command succeeds (exit code 0), the URL is accessible and contains valid JSON if [ $? -eq 0 ]; then echo "URL is accessible and contains valid JSON" else echo "Error: URL is either inaccessible or does not contain valid JSON" fiLength of output: 206
pkg/utils/file_utils.go (3)
5-5: LGTM: Import addition is correct and necessary.The addition of the
net/urlpackage import is appropriate for the new URL-related functions introduced in this file.
179-182: LGTM: IsURL function is well-implemented.The
IsURLfunction correctly checks if a string is a valid URL by verifying the presence of both a scheme and a host. The implementation is concise and handles potential errors fromurl.Parse.
Line range hint
1-195: Overall assessment: Changes align well with PR objectives.The additions to this file, namely the
IsURLandGetFileNameFromURLfunctions, provide essential utility for handling remote URLs. These changes directly support the PR's objective of adding support for remote schemas in manifest validation.The implementations are correct and follow good coding practices. They lay a solid foundation for the broader changes required to support remote schema references in the
atmostool.A minor suggestion was made to improve error handling in the
GetFileNameFromURLfunction for edge cases involving trailing slashes in URLs. Implementing this suggestion would further enhance the robustness of the remote schema support.internal/exec/validate_stacks.go (1)
4-6: Imports are appropriate and necessaryThe added imports for
"context","os", and"github.com/hashicorp/go-getter"are necessary for the new functionality of downloading the schema from a URL and have been correctly included.Also applies to: 11-11
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- internal/exec/validate_stacks.go (2 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/validate_stacks.go (1)
Learnt from: haitham911 PR: cloudposse/atmos#731 File: internal/exec/validate_stacks.go:93-98 Timestamp: 2024-10-20T00:41:57.135Z Learning: When downloading schema files in `internal/exec/validate_stacks.go`, use a consistent temporary file name to overwrite the file each time and avoid creating multiple temporary files.
|
@aknysh what do you think about making this the default location for the schema? That way |
I think yes, we can do it |
I think they can set the path to But at a very least, we should document how to disable it. And also update the documentation to indicate what is the default behavior. |
|
@haitham911 also please update docs here: https://atmos.tools/core-concepts/validate/json-schema with how to specify remote schemas |
|
@haitham911 please post screenshot of this in action. Please confirm that using files as well as remote URLs both work. Also, this PR is conflicted. |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- internal/exec/validate_stacks.go (2 hunks)
- pkg/utils/file_utils.go (2 hunks)
- website/docs/core-concepts/validate/json-schema.mdx (1 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/validate_stacks.go (2)
Learnt from: haitham911 PR: cloudposse/atmos#731 File: internal/exec/validate_stacks.go:93-98 Timestamp: 2024-10-20T00:41:57.135Z Learning: When downloading schema files in `internal/exec/validate_stacks.go`, use a consistent temporary file name to overwrite the file each time and avoid creating multiple temporary files.Learnt from: haitham911 PR: cloudposse/atmos#731 File: internal/exec/validate_stacks.go:0-0 Timestamp: 2024-10-20T00:57:53.500Z Learning: In `internal/exec/validate_stacks.go`, when downloading the Atmos JSON Schema file to the temp directory, the temporary file is overwritten each time, so explicit removal is not necessary.
🔇 Additional comments (2)
pkg/utils/file_utils.go (1)
204-218: Great job implementing the suggested improvement!The
GetFileNameFromURLfunction has been implemented correctly, addressing the concerns raised in the previous review. Specifically:
- It properly handles URL parsing and extraction of the file name.
- It now includes a check for edge cases where the extracted file name is "/" or ".", returning an error in these cases.
This implementation ensures robust handling of various URL formats and edge cases. Well done!
internal/exec/validate_stacks.go (1)
94-119: Security considerations when downloading remote schemasThe previous comment regarding adding security measures when downloading remote schemas is still applicable to this code segment.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- website/docs/core-concepts/validate/json-schema.mdx (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
website/docs/core-concepts/validate/json-schema.mdx (1)
49-54: LGTM! The schema URL is correctly specified.The example uses the correct production URL for the Atmos manifest schema, which aligns with the standardization goals.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- internal/exec/validate_stacks.go (3 hunks)
- pkg/utils/file_utils.go (2 hunks)
- website/docs/core-concepts/validate/json-schema.mdx (1 hunks)
🧰 Additional context used
📓 Learnings (1)
pkg/utils/file_utils.go (1)
Learnt from: osterman PR: cloudposse/atmos#731 File: pkg/utils/file_utils.go:198-202 Timestamp: 2024-10-23T20:13:23.054Z Learning: In `pkg/utils/file_utils.go`, the current implementation of the `IsURL` function is considered sufficient; avoid suggesting more complex URL validation in future reviews.
🔇 Additional comments (2)
pkg/utils/file_utils.go (2)
216-233: LGTM! Well-implemented URL filename extraction.The function is well-structured with proper error handling and edge cases:
- Empty URL validation
- URL parsing error handling
- Edge cases for "/" and "." filenames
198-214:⚠️ Potential issueReorder error handling in IsURL function.
The error check should be performed immediately after
url.Parseto prevent potential nil pointer dereference when accessingurl.Scheme.Apply this diff to fix the error handling order:
func IsURL(s string) bool { url, err := url.Parse(s) + if err != nil { + return false + } validSchemes := []string{"http", "https"} schemeValid := false for _, scheme := range validSchemes { if url.Scheme == scheme { schemeValid = true break } } - if err != nil { - return false - } return schemeValid }⛔ Skipped due to learnings
Learnt from: osterman PR: cloudposse/atmos#731 File: pkg/utils/file_utils.go:198-202 Timestamp: 2024-10-23T20:13:23.054Z Learning: In `pkg/utils/file_utils.go`, the current implementation of the `IsURL` function is considered sufficient; avoid suggesting more complex URL validation in future reviews.
aknysh
added
minor
aknysh
changed the title
aknysh
changed the title
|
These changes were released in v1.97.0. |
|
@haitham911 we haven't set a default schema. If you run: Full output |
What
atmosfor manifest validationschemasconfiguration to allow referencing remote schema files, e.g.:Why
References
atmos validate stacks
Summary by CodeRabbit
Summary by CodeRabbit
New Features
atmos.yamlconfiguration file.Documentation
Bug Fixes