Support GITHUB_TOKEN for HTTP Requests to github.com
#912
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhancements to GitHub-related utility functions across multiple files, focusing on improving GitHub token authentication and detection mechanisms. The changes primarily revolve around creating a more flexible GitHub client that can handle authenticated and unauthenticated requests and implementing custom URL detection for GitHub repositories. The modifications aim to provide better support for GitHub token usage in various operations like vendoring, version checking, and remote configuration fetching. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (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
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 (2)
pkg/utils/github_utils.go (2)
15-19: Consider logging when no token is found.
A small warning could clarify that requests will proceed unauthenticated. Mind potential security trade-offs in logging tokens if not handled carefully.
20-27: Potential opportunity for reusability.
If this function is called frequently under the same context, consider caching the client to avoid overhead from creating multiple clients.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/utils/github_utils.go(1 hunks)
🔇 Additional comments (5)
pkg/utils/github_utils.go (5)
5-5: Import looks good.
No issues with using the "os" package here.
9-9: Dependency import is appropriate.
No issues with "golang.org/x/oauth2" for handling token-based authentication.
12-13: Clear documentation.
Your docstring effectively communicates the purpose of the function.
14-14: Function signature is well-defined.
The context parameter promotes flexible usage of cancellation and timeouts.
36-36: Nice integration.
Replacing direct client creation withnewGitHubClientaligns well with the new pattern, enhancing consistency.
|
The most common use-case for fetching from GitHub is with vendoring. Anywhere we use e.g. Use this to conditionally add a header (e.g. |
osterman
changed the title
GITHUB_TOKEN for HTTP Requests to github.com for version command
osterman
changed the title
GITHUB_TOKEN for HTTP Requests to github.com for version commandGITHUB_TOKEN for HTTP Requests to github.com
|
💥 This pull request now has conflicts. Could you fix it @Listener430? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/exec/vendor_model.go (2)
239-251: RelocateroundTripperWithHeaderto a shared packageTo reuse the custom round tripper across the codebase, consider moving
roundTripperWithHeaderto a common utility package.
293-306: Extend GitHub host detection for custom domainsThe current host check only matches
github.com. To support GitHub Enterprise domains, consider using a more flexible condition.Update the condition:
- if githubToken != "" && strings.EqualFold(srcURL.Host, "github.com") { + if githubToken != "" && strings.Contains(srcURL.Host, "github.") {internal/exec/validate_stacks.go (1)
382-390: UseGITHUB_TOKENfor schema downloadsInclude the
GITHUB_TOKENwhen downloading the schema to handle authentication and rate limits.Adjust the client initialization:
+ githubToken := os.Getenv("GITHUB_TOKEN") + var httpClient *http.Client + if githubToken != "" && strings.Contains(manifestURL, "github.com") { + httpClient = &http.Client{ + Transport: roundTripperWithHeader{ + headers: map[string]string{ + "Authorization": fmt.Sprintf("Bearer %s", githubToken), + }, + }, + } + } else { + httpClient = http.DefaultClient + } + client := &getter.Client{ + Getters: []getter.Getter{ + &getter.HttpGetter{Client: httpClient}, + }, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
examples/demo-vendoring/vendor.yaml(1 hunks)go.mod(2 hunks)internal/exec/validate_stacks.go(2 hunks)internal/exec/vendor_model.go(4 hunks)internal/exec/vendor_model_component.go(3 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/vendor_model_component.go (1)
Learnt from: aknysh
PR: cloudposse/atmos#768
File: internal/exec/vendor_model_component.go:3-20
Timestamp: 2024-11-18T13:59:10.824Z
Learning: When replacing significant dependencies like `go-getter` that require extensive changes, prefer to address them in separate PRs.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
go.mod (2)
53-53: LGTM! OAuth2 package addition.The addition of
golang.org/x/oauth2aligns perfectly with the PR objective of implementing GitHub token support.
28-28: Verify go-getter v2 compatibility.The upgrade from v1 to v2 is a major version change that might have breaking changes.
Let's verify the impact:
✅ Verification successful
✅ go-getter v2 upgrade verified
The codebase consistently uses go-getter v2 with proper API patterns. No compatibility issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for go-getter usage patterns that might need updates ast-grep --pattern 'getter.$$$' # Also check for any v1 imports that might have been missed rg --type go "github.com/hashicorp/go-getter\b" -A 2Length of output: 2289
Listener430
force-pushed
the
DEV-2778-version
branch
from
48baf08 to
3a5147f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exec/vendor_utils.go (1)
721-730: Consider adding error handling for detector registration.While the implementation is correct, consider adding error handling for edge cases where detector registration might fail.
func RegisterCustomDetectors() { + // Ensure we don't add duplicate detectors + for _, d := range getter.Detectors { + if _, ok := d.(*CustomGitHubDetector); ok { + return + } + } getter.Detectors = append( []getter.Detector{ &CustomGitHubDetector{}, }, getter.Detectors..., ) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/exec/validate_stacks.go(1 hunks)internal/exec/vendor_model.go(1 hunks)internal/exec/vendor_model_component.go(2 hunks)internal/exec/vendor_utils.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/vendor_model_component.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (6)
internal/exec/vendor_utils.go (4)
672-676: LGTM! Valid scheme list updated correctly.The addition of
git::httpsto the valid schemes list is necessary to support the new GitHub URL transformation feature.
681-684: Well-documented custom detector implementation.The
CustomGitHubDetectorimplementation follows the go-getter's detector interface pattern and includes clear documentation about its purpose.Also applies to: 686-687
688-694: Robust URL handling and validation.The implementation properly handles:
- Empty source URLs
- URLs without schemes (defaulting to HTTPS)
710-713: Secure token handling implementation.The code securely:
- Retrieves the token from environment variable
- Uses URL's built-in methods for encoding credentials
internal/exec/validate_stacks.go (1)
382-382: LGTM! Detector registration properly placed.The
RegisterCustomDetectors()call is correctly placed before the client initialization.internal/exec/vendor_model.go (1)
271-274: LGTM! Detector registration properly placed.The
RegisterCustomDetectors()call is correctly placed before the client initialization.
|
Note, tests are failing due to intermittent issue: |
|
We also support remote schemas. We should make sure GitHub token is passed on those requests. |
|
Let’s create a new core section of atmos.yaml with a |
@osterman thanks, could you please share an example of "remote schemas" and what should be happening then? |
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
website/docs/cli/configuration/configuration.mdx (1)
164-164: LGTM! Consider adding a link to the environment variables section.The configuration setting is well-placed under the
settingssection. However, it would be helpful to add a reference to the corresponding environment variableATMOS_GITHUB_TOKENdocumented in the environment variables section.- inject_github_token: true # Adds the GITHUB_TOKEN as a Bearer token for GitHub API requests. + inject_github_token: true # Adds the GITHUB_TOKEN as a Bearer token for GitHub API requests. Can be overridden by ATMOS_GITHUB_TOKEN environment variable.internal/exec/vendor_utils.go (2)
716-736: Simplify token selection logic.The token selection logic could be more concise and clearer. Consider using a helper function to encapsulate the token selection logic.
-tokenATMOS := os.Getenv("ATMOS_GITHUB_TOKEN") -tokenGitHub := os.Getenv("GITHUB_TOKEN") - -var usedToken string -var tokenSource string - -// 1. If ATMOS_GITHUB_TOKEN is set, always use that -if tokenATMOS != "" { - usedToken = tokenATMOS - tokenSource = "ATMOS_GITHUB_TOKEN" - u.LogDebug(d.AtmosConfig, fmt.Sprintf("ATMOS_GITHUB_TOKEN is set; using it for %s\n", src)) -} else { - // 2. Otherwise, only inject GITHUB_TOKEN if cfg.Settings.InjectGithubToken == true - if d.AtmosConfig.Settings.InjectGithubToken && tokenGitHub != "" { - usedToken = tokenGitHub - tokenSource = "GITHUB_TOKEN" - u.LogTrace(d.AtmosConfig, fmt.Sprintf("InjectGithubToken=true and GITHUB_TOKEN is set, using it for %s\n", src)) - } else { - u.LogTrace(d.AtmosConfig, fmt.Sprintf("No ATMOS_GITHUB_TOKEN found or GITHUB_TOKEN unavailable. Won't inject for %s\n", src)) - } -} +token, source := selectGitHubToken(d.AtmosConfig) +if token != "" { + u.LogDebug(d.AtmosConfig, fmt.Sprintf("%s is set; using it for %s\n", source, src)) +} else { + u.LogTrace(d.AtmosConfig, fmt.Sprintf("No suitable GitHub token found. Won't inject for %s\n", src)) +}Add this helper function:
func selectGitHubToken(config schema.AtmosConfiguration) (token, source string) { if t := os.Getenv("ATMOS_GITHUB_TOKEN"); t != "" { return t, "ATMOS_GITHUB_TOKEN" } if config.Settings.InjectGithubToken { if t := os.Getenv("GITHUB_TOKEN"); t != "" { return t, "GITHUB_TOKEN" } } return "", "" }
755-764: LGTM! Consider adding more detailed documentation.The implementation correctly prepends the custom detector. Consider adding more detailed documentation about when this function should be called and its thread-safety characteristics.
-// RegisterCustomDetectors prepends the custom detector so it runs before -// the built-in ones. Any code that calls go-getter should invoke this. +// RegisterCustomDetectors prepends the CustomGitHubDetector to the list of go-getter +// detectors, ensuring it runs before built-in detectors. This function should be called +// before any go-getter operations that might involve GitHub URLs. +// +// Note: This function modifies the global getter.Detectors slice and should be called +// with appropriate synchronization in concurrent contexts. func RegisterCustomDetectors(atmosConfig schema.AtmosConfiguration) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/vendor_utils.go(2 hunks)website/docs/cli/configuration/configuration.mdx(3 hunks)
🧰 Additional context used
📓 Learnings (1)
website/docs/cli/configuration/configuration.mdx (1)
Learnt from: osterman
PR: cloudposse/atmos#912
File: website/docs/cli/configuration/configuration.mdx:113-114
Timestamp: 2025-01-09T20:02:37.990Z
Learning: The `inject_github_token` setting should be placed under the top-level `settings` section in `atmos.yaml` instead of creating a new `core` section.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
website/docs/cli/configuration/configuration.mdx (2)
200-204: LGTM! Documentation is clear and comprehensive.The documentation for the
inject_github_tokensetting is well-structured and provides clear information about:
- The purpose of the setting
- The behavior with
GITHUB_TOKEN- The override capability with
ATMOS_GITHUB_TOKEN
675-675: LGTM! Environment variables are well documented.The environment variables table has been updated with:
- The JSON Schema manifest path
- Version check configuration
- GitHub token configuration
All entries follow the established format and provide clear descriptions.
Also applies to: 679-680
internal/exec/vendor_utils.go (3)
14-14: LGTM! Import added for go-getter package.The addition of the go-getter package is appropriate for implementing GitHub token support.
672-676: LGTM! Added support for git::https scheme.The addition of "git::https" to valid schemes is necessary for supporting authenticated git cloning operations.
694-697: 🛠️ Refactor suggestionAdd URL validation before scheme injection.
The current implementation should validate the URL structure more thoroughly before prepending 'https://'.
if !strings.Contains(src, "://") { + // Validate URL format before processing + if strings.Contains(src, "..") || strings.Contains(src, "\\") { + return "", false, fmt.Errorf("invalid URL format: %s", src) + } u.LogDebug(d.AtmosConfig, fmt.Sprintf("No scheme in %q, prepending 'https://'", src)) src = "https://" + src }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/exec/vendor_utils.go (1)
715-735: Consider environment variable documentation.The code handles both
ATMOS_GITHUB_TOKENandGITHUB_TOKEN, but users might not be aware of this hierarchy.Consider adding a code comment explaining the token precedence:
+ // Token precedence: + // 1. ATMOS_GITHUB_TOKEN (if set, always used) + // 2. GITHUB_TOKEN (only used if InjectGithubToken=true) atmosGitHubToken := os.Getenv("ATMOS_GITHUB_TOKEN") gitHubToken := os.Getenv("GITHUB_TOKEN")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/vendor_utils.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
internal/exec/vendor_utils.go (4)
14-14: LGTM! Import of go-getter package.The addition of the go-getter package import aligns with the implementation of GitHub token support.
672-676: LGTM! Valid URI scheme extension.The addition of
git::httpsto valid schemes is correct and necessary for supporting authenticated git operations.
753-762: LGTM! Custom detector registration.The implementation correctly prepends the custom detector to ensure it runs before built-in detectors.
737-746: Verify token injection behavior.Let's verify that token injection behaves correctly when credentials are already present in the URL.
✅ Verification successful
Token injection implementation verified and secure.
The token injection behavior is correctly implemented with proper credential checks. The codebase contains a single, consistent implementation that safely handles existing credentials and follows GitHub's token authentication pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing token handling patterns in the codebase # Look for URL patterns with embedded credentials rg -n "://.*:.*@.*github\.com" # Look for other token injection implementations ast-grep --pattern 'if $token != "" { $$$ parsedURL.User = $$$ }'Length of output: 104
Script:
#!/bin/bash # Search for URL parsing and credential handling patterns ast-grep --pattern 'parsedURL := url.Parse($_)' # Look for URL user/password checks ast-grep --pattern 'parsedURL.User.Username()' # Search for token injection patterns with any variable name ast-grep --pattern 'parsedURL.User = url.UserPassword($_, $_)' # Find URL credential handling patterns rg -n "User = url\.UserPassword\(" -A 2Length of output: 844
|
These changes were released in v1.146.0. |
what
After the internal discussion and descoping of this PR there 2 parts left:
the support of the github token for the version command. The code checks if the token is set (e.g. via an env var) and if it is, the requests are done using the token. In case no token is provided, the requests to github are made without it.
for github links to folders and in case the ATMOS_GITHUB_TOKEN or GITHUB_TOKEN variable is set, the files are downloaded with go-getter (v1) that uses the token. The token is put into the url under the hood, so for the end user/custmer the interaction/syntax remains the same. I.e. the following commands specified in the vendor.yaml pull out the files from github using the token (not subject to anonymous users rate limits)
NB. github.com/analitikasi/Coonector.git - is a private repo
- component: "weather"
source: "github.com/analitikasi/Coonector.git//quick-start-simple/components/terraform/{{ .Component }}?ref={{.Version}}"
version: "main"
targets:
- "components/terraform/{{ .Component }}/{{.Version}}"
tags:
- demo
why
Bypass github ratelimits for non-auth requests (use a token instead)
references
GITHUB_TOKENfor HTTP Requests to github.com #871Summary by CodeRabbit
Summary by CodeRabbit
New Features
ATMOS_GITHUB_TOKENfor Bearer token usage in GitHub API requests.Improvements
Documentation
inject_github_tokenconfiguration option inatmos.yaml.