Support GITHUB_TOKEN for HTTP Requests to github.com
#871
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhanced support for GitHub source handling in Atmos, focusing on improving the vendor command's ability to download files directly from GitHub repositories. New utility functions for parsing GitHub URLs and downloading files are added, enabling more flexible configuration management. The modifications include updating the source type determination logic, introducing temporary directory handling, and improving error management for GitHub-based sources. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
Documentation and Community
|
![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 (6)
internal/exec/vendor_utils.go (3)
268-269: Consider limiting the scope of the global variable.Declaring "tempDir" at the package level or near global scope might be avoided if you can define it strictly where it’s used. Reducing scope simplifies testing and prevents accidental misuse.
371-388: Handle edge cases in GitHub file downloading.This logic for saving downloaded content to a temp directory looks fine. Consider validating that the file path from GitHub won't collide with local paths. Retries might also help if the download fails.
526-551: Refine the multiple-return boolean approach.Returning four booleans can be confusing. Consider introducing a small struct or an enum-like pattern to improve clarity and reduce the likelihood of confusion at the call site.
pkg/utils/github_utils.go (1)
37-69: Gracefully handle connection or rate limit errors.The download function is straightforward. For better resilience, consider handling transient network failures with retries or more descriptive messages for rate limit errors.
pkg/vender/vendor_config_test.go (1)
146-195: Add negative test scenarios.This test successfully ensures the flow works with a valid token. Adding scenarios to verify behavior when the token is missing or invalid might strengthen coverage.
pkg/utils/file_utils.go (1)
246-268: Consider handling GitHub Enterprise URLs.The function parses GitHub URLs in standard blob format. For broader use, consider supporting GitHub Enterprise domains or “raw” URLs. This might improve generality.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/exec/vendor_utils.go(4 hunks)pkg/utils/file_utils.go(1 hunks)pkg/utils/github_utils.go(2 hunks)pkg/vender/vendor_config_test.go(1 hunks)
🔇 Additional comments (1)
internal/exec/vendor_utils.go (1)
319-328: Creation of a temp directory is correct.
Good job creating a temporary directory and deferring its cleanup. This approach helps avoid leaving junk files behind. No further changes needed.
osterman
changed the title
GITHUB_TOKEN for HTTP Requests to github.com
Listener430
force-pushed
the
DEV-2778
branch
from
5185fc0 to
61df3aa
Compare
Listener430
force-pushed
the
DEV-2778
branch
from
3f79124 to
22f6314
Compare
Listener430
force-pushed
the
DEV-2778
branch
from
ea78b85 to
65b4c92
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
pkg/config/config.go (2)
19-19: Consolidate duplicate imports forutils.You have imported the same package twice, once as
utilsand once aliased asu. If both references are needed, remove one import statement to keep it consistent.19c19 - "github.com/cloudposse/atmos/pkg/utils" - u "github.com/cloudposse/atmos/pkg/utils" + u "github.com/cloudposse/atmos/pkg/utils"
403-432: Consider extending support beyond GitHub.
processRemoteConfigFilecurrently only handlesgh.neting.cc. If you foresee using GitHub Enterprise or other Git servers, consider refactoring to support different domains.cmd/terraform.go (1)
49-70: Warn about potential file overwrite.When writing the downloaded file to disk, consider checking whether the file already exists to avoid unexpected overwrites.
+if _, err := os.Stat(localPath); err == nil { + // Optionally log or prompt before overwriting +} err = os.WriteFile(localPath, data, 0o644)pkg/utils/github_utils.go (3)
32-46: The 2-second timeout might be too strict.For slower networks or large release data, consider an environment-based or configurable timeout.
52-86: Five-second timeout may be short for bigger files.Downloading large files from GitHub might need a slightly longer (or configurable) timeout. Otherwise, this function is clear.
88-95: Support for GitHub Enterprise is currently omitted.
IsGithubURLonly matchesgh.neting.cc. If GitHub Enterprise usage is likely, consider generalizing the URL check.pkg/vender/vendor_config_test.go (1)
147-196: Test coverage for token usage is a welcome addition.You could add a negative test (e.g. invalid token) to confirm proper error handling.
pkg/utils/file_utils.go (1)
246-265: LGTM! Consider enhancing error messages.The function correctly parses GitHub URLs and handles both 'blob' and 'raw' formats. The implementation is clean and efficient.
Consider enhancing error messages to include the actual URL for better debugging:
- return "", "", "", "", fmt.Errorf("invalid GitHub URL format") + return "", "", "", "", fmt.Errorf("invalid GitHub URL format: %s", rawURL)internal/exec/vendor_utils.go (1)
533-561: Consider breaking down the function for better maintainability.While the implementation is correct, the function is handling multiple responsibilities.
Consider splitting into smaller, focused functions:
func determineSourceType(uri *string, vendorConfigFilePath string) (bool, bool, bool, bool) { + useOciScheme := isOciScheme(uri) + useLocalFileSystem, sourceIsLocalFile := checkLocalFileSystem(uri, vendorConfigFilePath) + isGitHubSource := checkGitHubSource(uri) + return useOciScheme, useLocalFileSystem, sourceIsLocalFile, isGitHubSource +} + +func isOciScheme(uri *string) bool { + useOciScheme := strings.HasPrefix(*uri, "oci://") + if useOciScheme { + *uri = strings.TrimPrefix(*uri, "oci://") + } + return useOciScheme +} + +func checkLocalFileSystem(uri *string, vendorConfigFilePath string) (bool, bool) { + parsedURL, err := url.Parse(*uri) + if err != nil || parsedURL.Scheme == "" || parsedURL.Host == "" { + if absPath, err := u.JoinAbsolutePathWithPath(vendorConfigFilePath, *uri); err == nil { + *uri = absPath + return true, u.FileExists(*uri) + } + } + return false, false +} + +func checkGitHubSource(uri *string) bool { + parsedURL, err := url.Parse(*uri) + return err == nil && parsedURL.Host == "github.com" && parsedURL.Scheme == "https" }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/terraform.go(3 hunks)internal/exec/stack_processor_utils.go(1 hunks)internal/exec/utils.go(1 hunks)internal/exec/vendor_utils.go(4 hunks)pkg/config/config.go(5 hunks)pkg/utils/file_utils.go(1 hunks)pkg/utils/github_utils.go(1 hunks)pkg/vender/vendor_config_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/exec/stack_processor_utils.go
🔇 Additional comments (11)
pkg/config/config.go (3)
7-7: Good addition of the net/url import.
The usage for parsing remote configurations seems straightforward.
105-106: Thanks for documenting environment variables.
The comments clarify the order of configuration resolution using ENV vars.
207-218: Remote config loading looks robust.
The logic correctly checks for the remote config variable, attempts retrieval, and returns errors appropriately.
cmd/terraform.go (2)
4-6: Neat addition of imports for file handling and string processing.
No issues found with these imports.
83-87: Documentation for --folder is concise.
This new flag is self-explanatory and aligns well with the new GitHub download feature.
internal/exec/utils.go (1)
24-24: The --folder flag inclusion looks consistent.
This aligns with the logic in cmd/terraform.go.
pkg/utils/github_utils.go (3)
5-8: Import updates for GitHub support look appropriate.
These libraries are well-chosen for encoding, URL parsing, etc.
12-12: OAuth2 import is essential for authenticated requests.
No changes are necessary.
15-30: Authentication fallback is a helpful approach.
This gracefully handles both token and non-token scenarios.
internal/exec/vendor_utils.go (2)
271-272: LGTM! Proper temporary directory handling.
The implementation follows best practices for temporary file management with proper cleanup and error handling.
Also applies to: 322-331
376-395: LGTM! Secure and well-structured GitHub source handling.
The implementation properly handles GitHub sources with appropriate error handling and logging. The code follows security best practices by validating URLs before using tokens.
| // ParseFilenameFromURL extracts the file name from a URL | ||
| func ParseFilenameFromURL(url string) string { | ||
| parts := strings.Split(url, "/") | ||
| if len(parts) == 0 { | ||
| return "" | ||
| } | ||
| return parts[len(parts)-1] // e.g. "dev.yaml" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and error handling for robustness.
The function should validate input and handle edge cases properly.
Consider this more robust implementation:
-func ParseFilenameFromURL(url string) string {
+func ParseFilenameFromURL(rawURL string) (string, error) {
+ if rawURL == "" {
+ return "", fmt.Errorf("empty URL provided")
+ }
+
+ parsedURL, err := url.Parse(rawURL)
+ if err != nil {
+ return "", fmt.Errorf("invalid URL: %w", err)
+ }
+
parts := strings.Split(parsedURL.Path, "/")
if len(parts) == 0 {
- return ""
+ return "", fmt.Errorf("URL has no path components: %s", rawURL)
}
- return parts[len(parts)-1] // e.g. "dev.yaml"
+ filename := parts[len(parts)-1]
+ if filename == "" {
+ return "", fmt.Errorf("URL ends with a slash: %s", rawURL)
+ }
+ return filename, nil
}📝 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.
| // ParseFilenameFromURL extracts the file name from a URL | |
| func ParseFilenameFromURL(url string) string { | |
| parts := strings.Split(url, "/") | |
| if len(parts) == 0 { | |
| return "" | |
| } | |
| return parts[len(parts)-1] // e.g. "dev.yaml" | |
| } | |
| // ParseFilenameFromURL extracts the file name from a URL | |
| func ParseFilenameFromURL(rawURL string) (string, error) { | |
| if rawURL == "" { | |
| return "", fmt.Errorf("empty URL provided") | |
| } | |
| parsedURL, err := url.Parse(rawURL) | |
| if err != nil { | |
| return "", fmt.Errorf("invalid URL: %w", err) | |
| } | |
| parts := strings.Split(parsedURL.Path, "/") | |
| if len(parts) == 0 { | |
| return "", fmt.Errorf("URL has no path components: %s", rawURL) | |
| } | |
| filename := parts[len(parts)-1] | |
| if filename == "" { | |
| return "", fmt.Errorf("URL ends with a slash: %s", rawURL) | |
| } | |
| return filename, nil | |
| } |
| @@ -100,6 +102,8 @@ func InitCliConfig(configAndStacksInfo schema.ConfigAndStacksInfo, processStacks | |||
| // system dir (`/usr/local/etc/atmos` on Linux, `%LOCALAPPDATA%/atmos` on Windows) | |||
| // home dir (~/.atmos) | |||
| // current directory | |||
| // ENV var ATMOS_CLI_CONFIG_PATH | |||
| // ENV var ATMOS_REMOTE_CONFIG_URL from GITHUB | |||
There was a problem hiding this comment.
This task should not be implementing a new ENV for fetching remote config.
what
This PR contains the code that checks if GITHUB_TOKEN env variable is set, and if it does, the requests to github.com are done with the provided token.
The following functionality was added :
why
Bypass github ratelimts and allow access to the files in the private repos.
Testing
atmos.yaml local vs. remote
This testcase is based on the green update box that's shown depedning on the settings from the atmos.yaml file.
The local atmos yaml version has this defaulted to 1 h (see section A below), i.e. if one runs the version command in less than a hour the update box isn't shown (sections B and C at the screenshot below).
Then a slightly adjusted version of atmos.yaml (see section D below, the timeout is set to a smaller value vs 1000 ms in the original local file above, i.e. if applied correctly the greenbox should be shown every time a version command is run)
Since GITHUB_TOKEN isn't yet set, the private repo isn't available.
Then ATMOS_REMOTE_CONFIG_URL and GITHUB_TOKEN env variables are set (see section E below),
and in section F the version command is run twice and the greenbox is shown everytime (i.e. the updated timeout setting from the remote repo is respected)
NB. This test case also covers the version command that honors GITHUB_TOKEN value too.
remote vendoring
Make sure that GITHUB_TOKEN variable is empty brefore running this test.
In the vendoring example, a new component is added. The component is located in the private repo, it is a standard vendor.yaml from demo-vendoring example with the following extra section (shown in section A at the screenshot below)
`
source: "https://github.com/analitikasi/CoonectorDescribed/blob/main/README.md"
version: "main"
targets:
- "components/docs/{{ .Component }}/{{ .Version}}"
tags:
- demo
- github
`
Then vednor pull command is run that fails as the repo is private (see section B at the screenshot above).
Also there were no files added to components folder (refer to section C at the screenshot above)
Then the token variable is assigned as per section D at the screenshot below
The command succesfully completes (see section E) and the file from the private repo is downloaded into the specified locaion (F)
local stacks vs remote stacks
The test is based on quick start simple example, i.e. first it is run with local stacks on (shows weather for stockholm), then is it run with a remote stack (boston). There's a new flag added for remote stack uploading to specify the folder where it should land once downloaded. E.g.
./../../atmos terraform apply station -s https://github.com/analitikasi/CoonectorDescribed/blob/main/quick-start-simple/stacks/deploy/dev.yaml --folder ./stacks/deployHere is the local stack file from the sample
Here is the output when run with local stack
After that, a remote stack is checked, it is located in a private repo and looks like this (note it says Boston unlike the value from the local stack so it is possible to differenciate local vs remote stacks)
and since GITHUB_TOKEN variable isn't set it results in 404 error:
If the token is set, the output looks like this
Please note that the new flag was introduced to specify the location where the remote stacks have to be downloaded to.
references
Link to ticket - https://linear.app/cloudposse/issue/DEV-2778/atmos-should-support-github-token-for-http-requests-to-githubcom
Summary by CodeRabbit
Summary by CodeRabbit
--folderflag to specify the download location for stack files.