[WEB-4013]chore: publish login and standardize urls in common settings #7013
[WEB-4013]chore: publish login and standardize urls in common settings #7013sriramveeraghanta merged 4 commits intopreviewfrom
Conversation
|
Pull Request Linked with Plane Work Items Comment Automatically Generated by Plane |
|
""" WalkthroughThe changes introduce URL validation and default path assignments for several base URL configuration variables in the settings. A new utility function, Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment Variables
participant Settings as common.py
participant Utils as is_valid_url / get_url_components
participant App as Application
Env->>Settings: Provide *_BASE_URL and *_BASE_PATH variables
Settings->>Utils: Validate each *_BASE_URL with is_valid_url
Utils-->>Settings: Return True/False for each URL
Settings->>Settings: Assign default *_BASE_PATH if missing
Settings->>Settings: If valid, concatenate LIVE_BASE_URL + LIVE_BASE_PATH to form LIVE_URL
App->>Settings: Use validated URLs and LIVE_URL
sequenceDiagram
participant Task as sync_with_external_service
participant Settings as common.py
participant Utils as get_url_components
participant ExternalService as External HTTP Service
Task->>Settings: Check LIVE_URL
alt LIVE_URL is valid
Task->>Utils: Parse LIVE_URL components
Utils-->>Task: Return URL components
Task->>ExternalService: POST to constructed URL (scheme + netloc + path + endpoint)
ExternalService-->>Task: Return response
else LIVE_URL is not set or invalid
Task-->>Task: Return empty dict, skip POST
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (3)
apiserver/plane/settings/common.py (3)
343-343: Improve URL construction for LIVE_URL.The current string concatenation method might lead to issues if
LIVE_BASE_URLends with a slash andLIVE_BASE_PATHstarts with a slash (double slash) or if neither has a slash (missing slash).Consider using
urllib.parse.urljoinfor more robust URL construction:-LIVE_URL = f"{LIVE_BASE_URL}{LIVE_BASE_PATH}" if LIVE_BASE_URL else None +from urllib.parse import urljoin +LIVE_URL = urljoin(LIVE_BASE_URL, LIVE_BASE_PATH) if LIVE_BASE_URL else None
317-343: Consider adding composite URLs for consistency.Only
LIVE_URLhas a composite variable created. For consistency and to avoid duplicating this logic elsewhere, consider creating similar composite variables for the other base URLs.ADMIN_BASE_PATH = os.environ.get("ADMIN_BASE_PATH", "/god-mode/") +ADMIN_URL = f"{ADMIN_BASE_URL}{ADMIN_BASE_PATH}" if ADMIN_BASE_URL else None SPACE_BASE_PATH = os.environ.get("SPACE_BASE_PATH", "/spaces/") +SPACE_URL = f"{SPACE_BASE_URL}{SPACE_BASE_PATH}" if SPACE_BASE_URL else None APP_BASE_PATH = os.environ.get("APP_BASE_PATH", "/") +APP_URL = f"{APP_BASE_URL}{APP_BASE_PATH}" if APP_BASE_URL else NoneIf you implement this along with the
urljoinsuggestion, you'd use the same pattern for all URLs.
321-341: Consider adding logging for invalid URLs.The code currently silently sets invalid URLs to
None. While this is a safe approach, adding logging would help administrators diagnose configuration issues.+import logging +logger = logging.getLogger(__name__) if ADMIN_BASE_URL and not is_valid_url(ADMIN_BASE_URL): + logger.warning(f"Invalid ADMIN_BASE_URL provided: {ADMIN_BASE_URL}") ADMIN_BASE_URL = None if SPACE_BASE_URL and not is_valid_url(SPACE_BASE_URL): + logger.warning(f"Invalid SPACE_BASE_URL provided: {SPACE_BASE_URL}") SPACE_BASE_URL = None if APP_BASE_URL and not is_valid_url(APP_BASE_URL): + logger.warning(f"Invalid APP_BASE_URL provided: {APP_BASE_URL}") APP_BASE_URL = None if LIVE_BASE_URL and not is_valid_url(LIVE_BASE_URL): + logger.warning(f"Invalid LIVE_BASE_URL provided: {LIVE_BASE_URL}") LIVE_BASE_URL = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apiserver/plane/authentication/utils/host.py(2 hunks)apiserver/plane/bgtasks/copy_s3_object.py(1 hunks)apiserver/plane/settings/common.py(2 hunks)apiserver/plane/utils/host.py(2 hunks)apiserver/plane/utils/url.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
apiserver/plane/bgtasks/copy_s3_object.py (1)
apiserver/plane/app/views/page/base.py (2)
post(431-436)post(570-623)
apiserver/plane/authentication/utils/host.py (1)
apiserver/plane/utils/host.py (1)
base_host(13-59)
apiserver/plane/utils/host.py (1)
apiserver/plane/authentication/utils/host.py (1)
base_host(12-55)
apiserver/plane/settings/common.py (1)
apiserver/plane/utils/url.py (1)
is_valid_url(5-26)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (13)
apiserver/plane/bgtasks/copy_s3_object.py (2)
71-73: Good addition of defensive programmingAdding this guard clause prevents making unnecessary HTTP requests when
LIVE_URLis not configured, which improves robustness and performance.
75-75: Change from LIVE_BASE_URL to LIVE_URL properly aligns with URL standardizationThis change utilizes the new composite URL setting that combines base URL and path with validation, providing a more standardized approach to URL configuration.
apiserver/plane/authentication/utils/host.py (2)
12-17: Style improvement for better readabilityReformatting the function signature from a single line to multiple lines improves code readability and maintainability.
22-22: Consistent terminology in commentsChanging from "redirections" to "redirection" maintains consistency in terminology across the codebase.
Also applies to: 35-35
apiserver/plane/utils/host.py (2)
13-18: Style improvement for better readabilityReformatting the function signature from a single line to multiple lines improves code readability and maintainability, consistent with changes in the authentication module.
26-26: Consistent terminology in commentsChanging from "redirections" to "redirection" maintains consistency in terminology across the codebase, matching the changes in the authentication module.
Also applies to: 39-39
apiserver/plane/utils/url.py (1)
5-26: Well-implemented URL validation utilityThis new utility function properly validates URLs by checking for the presence of both scheme and network location, with appropriate exception handling. The comprehensive docstring with examples is a good practice.
The function could be enhanced in the future to support more complex validation scenarios, but for the current use case of validating configuration URLs, this implementation is sufficient.
apiserver/plane/settings/common.py (6)
16-17: Good choice of utility function import.The imported
is_valid_urlfunction provides a clean way to validate URLs consistently throughout the settings.
317-318: Clean section header for improved readability.The addition of this section header clearly organizes the code and makes it easier to locate URL-related settings.
321-323: Robust URL validation for ADMIN_BASE_URL.The validation logic ensures that only properly formed URLs are used, preventing potential issues with malformed URLs.
327-329: Robust URL validation for SPACE_BASE_URL.The validation logic ensures that only properly formed URLs are used, preventing potential issues with malformed URLs.
332-335: Robust URL validation for APP_BASE_URL.The validation logic ensures that only properly formed URLs are used, preventing potential issues with malformed URLs.
338-341: Robust URL validation for LIVE_BASE_URL.The validation logic ensures that only properly formed URLs are used, preventing potential issues with malformed URLs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apiserver/plane/utils/url.py (1)
42-42: Consider breaking long docstring example line.The example line exceeds the project's line length limit (119 > 88 characters).
- >>> get_url_components("https://example.com/path?query=1") + >>> get_url_components("https://example.com/path?query=1") + # Returns:🧰 Tools
🪛 Ruff (0.8.2)
42-42: Line too long (119 > 88)
(E501)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apiserver/plane/bgtasks/copy_s3_object.py(3 hunks)apiserver/plane/utils/url.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apiserver/plane/bgtasks/copy_s3_object.py
🧰 Additional context used
🪛 Ruff (0.8.2)
apiserver/plane/utils/url.py
42-42: Line too long (119 > 88)
(E501)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
apiserver/plane/utils/url.py (2)
1-28: Well-implemented URL validation function with proper error handling.The
is_valid_urlfunction is well-structured with comprehensive documentation, proper type hints, and robust validation logic. The implementation correctly checks for both scheme and netloc components, which are essential for a valid URL, and includes error handling for potential TypeError exceptions.
30-55: Good URL component extraction with proper validation.The
get_url_componentsfunction effectively builds on the validation logic and properly parses URL components into a structured dictionary. The function correctly returns None for invalid URLs and includes comprehensive documentation.🧰 Tools
🪛 Ruff (0.8.2)
42-42: Line too long (119 > 88)
(E501)
makeplane#7013) * chore: handling base path and urls * chore: uniformize urls in common settings * correct live url * chore: use url join to correctly join urls --------- Co-authored-by: sriram veeraghanta <veeraghanta.sriram@gmail.com>
#7013) * chore: handling base path and urls * chore: uniformize urls in common settings * correct live url * chore: use url join to correctly join urls --------- Co-authored-by: sriram veeraghanta <veeraghanta.sriram@gmail.com>
3 participants
Description
chore: publish login and standardize urls in common settings
Type of Change
Test Scenarios
References
WEB-4013
Summary by CodeRabbit
New Features
Bug Fixes
Style