Skip to content

migrate validate_endpoint to python #2868

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

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

KyriosGN0
Copy link
Contributor

@KyriosGN0 KyriosGN0 commented Jun 17, 2025

User description

Signed-off-by: AvivGuiser [email protected]

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

migrate the validate_endpoint.sh to python

Motivation and Context

solves #2650 partially

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement


Description

• Migrate validate_endpoint.sh from Bash to Python
• Replace shell script with Python equivalent functionality
• Update video script to call Python version
• Maintain all existing endpoint validation features


Changes walkthrough 📝

Relevant files
Enhancement
validate_endpoint.sh
Remove Bash endpoint validation script                                     

Video/validate_endpoint.sh

• Complete removal of Bash script (30 lines deleted)
• Contained
endpoint validation logic with curl commands
• Handled authentication
and GraphQL endpoint checks

+0/-30   
video.sh
Update script call to Python version                                         

Video/video.sh

• Update function call from validate_endpoint.sh to
validate_endpoint.py
• Change shell script invocation to Python script
execution

+1/-1     
validate_endpoint.py
Add Python endpoint validation implementation                       

Video/validate_endpoint.py

• New Python script with 136 lines implementing endpoint validation

Includes timestamp formatting, HTTP session management, and
authentication
• Handles both regular and GraphQL endpoint validation

• Provides comprehensive error handling and status code checking

+136/-0 

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @KyriosGN0 KyriosGN0 marked this pull request as ready for review June 17, 2025 21:09
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    2650 - PR Code Verified

    Compliant requirements:

    • Convert language syntax while ensuring all functionalities are retained
    • No impact on current support features
    • User experience should remain unchanged

    Requires further human verification:

    • Migrate video recorder backend script control from Bash to Python (partial completion - only validate_endpoint migrated)

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Unused Session

    The create_session function creates a requests session but doesn't configure any timeout or retry logic, and the max_time parameter is not used in session configuration. The session could be simplified or enhanced with proper configuration.

    def create_session(max_time=1):
        """Create requests session with timeout configuration."""
        session = requests.Session()
        return session
    Timestamp Logic

    Complex timestamp formatting logic with microseconds to milliseconds conversion may not handle edge cases properly. The string manipulation for trimming microseconds could fail if timestamp format varies.

    if '%3N' in ts_format:
        # Replace %3N (bash milliseconds) with %f (Python microseconds) and trim later
        ts_format_python = ts_format.replace('%3N', '%f')
        timestamp = datetime.now(timezone.utc).strftime(ts_format_python)
        # Convert microseconds to milliseconds (trim last 3 digits)
        if ',%f' in ts_format:
            # Find the microseconds part and trim to milliseconds
            parts = timestamp.rsplit(',', 1)
            if len(parts) == 2 and len(parts[1]) == 6:
                timestamp = parts[0] + ',' + parts[1][:3]
    else:
        timestamp = datetime.now(timezone.utc).strftime(ts_format)
    return timestamp

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 17, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Fix timestamp format condition check
    Suggestion Impact:The suggestion was directly implemented - the condition was changed from checking ',%f' in ts_format to checking '%f' in ts_format_python, fixing the bug in timestamp formatting logic

    code diff:

    -        if ',%f' in ts_format:
    +        if '%f' in ts_format_python:

    The condition checks for ,%f in ts_format but should check for %f in
    ts_format_python since that's the format being used. This could cause incorrect
    timestamp formatting.

    Video/validate_endpoint.py [23-27]

    -if ',%f' in ts_format:
    +if '%f' in ts_format_python:
         # Find the microseconds part and trim to milliseconds
         parts = timestamp.rsplit(',', 1)
         if len(parts) == 2 and len(parts[1]) == 6:
             timestamp = parts[0] + ',' + parts[1][:3]

    [Suggestion processed]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies a bug in the timestamp formatting logic. The code checks for ,%f in the original ts_format string, but it should check for it in the modified ts_format_python string. This error would prevent the millisecond trimming logic from executing when intended, leading to incorrect timestamp formatting.

    Medium
    Security
    Prevent sensitive information exposure

    Using str(e) may expose sensitive information like authentication details or
    internal URLs in error messages. Consider using a generic error message or
    sanitizing the exception details.

    Video/validate_endpoint.py [98-100]

     except requests.exceptions.RequestException as e:
    -    print(f"{get_timestamp()} [{process_name}] - Error connecting to endpoint {endpoint}: {str(e)}")
    +    print(f"{get_timestamp()} [{process_name}] - Error connecting to endpoint {endpoint}: Request failed")
         return False
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies a potential security risk where printing the full exception str(e) might leak sensitive information in logs. Replacing it with a generic error message is a good practice for security hardening. The impact is significant as it mitigates a potential information leak.

    Medium
    Possible issue
    Configure session timeout properly
    Suggestion Impact:The commit addressed the core issue identified in the suggestion by removing the unused max_time parameter from create_session function, making the function signature consistent with its actual behavior

    code diff:

    -def create_session(max_time=1):
    +def create_session():
         """Create requests session with timeout configuration."""
         session = requests.Session()
         return session
    @@ -59,7 +59,7 @@
             max_time (int): Maximum time for request in seconds
         """
         process_name = "endpoint.checks"
    -    session = create_session(max_time)
    +    session = create_session()

    The max_time parameter is not being used in the session configuration. Configure
    the session with proper timeout settings to ensure requests respect the timeout
    value.

    Video/validate_endpoint.py [33-36]

     def create_session(max_time=1):
         """Create requests session with timeout configuration."""
         session = requests.Session()
    +    session.timeout = max_time
         return session

    [Suggestion processed]

    Suggestion importance[1-10]: 4

    __

    Why: The suggestion correctly points out that the max_time parameter in create_session is unused, which is misleading. While the timeout is correctly applied at the request level in session.get and session.post, setting it at the session level as suggested would improve code clarity and make the create_session function's signature and docstring accurate.

    Low
    • Update

    Signed-off-by: AvivGuiser <[email protected]>
    Signed-off-by: AvivGuiser <[email protected]>
    @KyriosGN0 KyriosGN0 requested a review from cgoldberg June 19, 2025 13:06
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants