-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Envs for Github Enterprise Oauth App #404
Conversation
WalkthroughThe changes introduce new environment variables for GitHub OAuth in the Changes
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
backend/.env.development (3)
16-19
: Enhance documentation for enterprise setup.While the comment explains the basic purpose, consider adding:
- A note about URL validation requirements
- A complete example for enterprise setup
# GITHUB_AUTHORIZATION_URL: Authorization URL for Github OAuth app. # Default OAuth app is Github. Change this url for using Github Enterprise OAuth App. # Format: https://ENTERPRISE_INSTANCE_URL/login/oauth/authorize +# Example for enterprise: https://github.mycompany.com/login/oauth/authorize +# Note: URL must use HTTPS protocol GITHUB_AUTHORIZATION_URL=https://github.com/login/oauth/authorize
20-23
: Fix inconsistent format documentation.The format example is missing the 'Format:' prefix, unlike other environment variables in this file.
# GITHUB_TOKEN_URL: Token URL for Github OAuth app. # Default OAuth app is Github. Change this url for using Github Enterprise OAuth App. -# https://ENTERPRISE_INSTANCE_URL/login/oauth/access_token +# Format: https://ENTERPRISE_INSTANCE_URL/login/oauth/access_token +# Example for enterprise: https://github.mycompany.com/login/oauth/access_token GITHUB_TOKEN_URL=https://github.com/login/oauth/access_token
24-27
: Add API version context and fix format documentation.The configuration should clarify the API version requirement and maintain consistent documentation format.
# GITHUB_USER_PROFILE_URL: User profile URL for Github OAuth app. # Default OAuth app is Github. Change this url for using Github Enterprise OAuth App. -# https://ENTERPRISE_INSTANCE_URL/api/v3/user +# Format: https://ENTERPRISE_INSTANCE_URL/api/v3/user +# Example for enterprise: https://github.mycompany.com/api/v3/user +# Note: The /api/v3 prefix is required for both GitHub and GitHub Enterprise GITHUB_USER_PROFILE_URL=https://api.github.com/user
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
backend/.env.development
(1 hunks)backend/src/auth/github.strategy.ts
(1 hunks)
🔇 Additional comments (1)
backend/src/auth/github.strategy.ts (1)
14-16
: Verify the configuration loading.
The current implementation assumes the configuration values will be available. Let's verify the environment variable setup across different environments.
✅ Verification successful
Based on the verification results, I can now provide a final response:
Environment configuration is properly set up and validated
The codebase shows proper environment configuration handling:
- Global configuration setup in
app.module.ts
with environment-specific file loading - Environment variables are properly validated using class-validator decorators
- Configuration is properly injected via ConfigService in the GitHub strategy
- Environment documentation exists in multiple .env.example files
- Test coverage exists for configuration-related code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for environment variable definitions across configuration files
# Look for GitHub OAuth URL configurations in environment files
echo "Checking environment files for GitHub OAuth URL configurations..."
rg -l "GITHUB_(AUTHORIZATION|TOKEN|USER_PROFILE)_URL" --type-add 'env:*.env*' --type env
# Check for any hardcoded GitHub URLs that might need updating
echo -e "\nChecking for potential hardcoded GitHub URLs..."
rg -i "https://(api\.)?github\.com" --type typescript
Length of output: 433
Script:
#!/bin/bash
# Let's try a different approach to find configuration files and GitHub-related URLs
# Search for env files with any extension
echo "Looking for environment files..."
fd -e env -e env.example -e env.sample -e env.template
# Search for GitHub-related environment variables in all files
echo -e "\nSearching for GitHub-related environment variables..."
rg "GITHUB_(AUTHORIZATION|TOKEN|USER_PROFILE)_URL"
# Search for GitHub URLs in all files (without type restriction)
echo -e "\nChecking for GitHub URLs in the codebase..."
rg -i "https://(api\.)?github\.com"
# Look for configuration setup in TypeScript files
echo -e "\nChecking for configuration setup in TypeScript files..."
fd -e ts -e tsx --exec grep -l "ConfigService\|ConfigModule"
Length of output: 26314
Script:
#!/bin/bash
# Let's check for environment variable configuration and validation
# Check for environment validation or configuration setup in the codebase
echo "Checking for environment validation..."
rg -A 10 "ConfigModule\.forRoot|validateSync|IsNotEmpty|IsString" backend/src/
# Check for environment variable documentation
echo -e "\nChecking for environment documentation..."
fd -e md -e env.example -e env.template --exec cat {}
# Check for environment-related tests
echo -e "\nChecking for environment-related tests..."
fd -e spec.ts -e test.ts --exec grep -l "ConfigService\|ConfigModule\|process.env"
Length of output: 51655
Thanks for the PR. Could you sign the CLA? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution.
@devleejb done! thank you :) |
What this PR does / why we need it:
Added an envs for using passport-github on Github Enterprise Oauth app.
Default value is for Github Oauth app. if yor want to Github Enterprise OAuth app, change those envs.
https://github.com/login/oauth/authorize
=>https://ENTERPRISE_INSTANCE_URL/login/oauth/authorize
https://github.com/login/oauth/access_token
=>https://ENTERPRISE_INSTANCE_URL/login/oauth/access_token
https://api.github.com/user
=>https://ENTERPRISE_INSTANCE_URL/api/v3/user
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
GithubStrategy
to utilize additional parameters for better integration with GitHub services.Bug Fixes
Documentation