Skip to content
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

Support rediss (redis+TLS) URLs #42

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

joey-squid
Copy link

@joey-squid joey-squid commented Jul 18, 2024

User description

redis-dump rediss://some-host Just Works.


PR Type

Enhancement


Description

  • Added support for rediss (Redis over TLS) URLs in both redis-dump and redis-load.
  • Updated URI matching logic to handle both redis and rediss schemes.

Changes walkthrough 📝

Relevant files
Enhancement
redis-dump
Add support for `rediss` (Redis over TLS) URLs in redis-dump

exe/redis-dump

  • Updated URI matching to support rediss scheme.
  • Ensured compatibility with both redis and rediss schemes.
  • +1/-1     
    redis-load
    Add support for `rediss` (Redis over TLS) URLs in redis-load

    exe/redis-load

  • Updated URI matching to support rediss scheme.
  • Ensured compatibility with both redis and rediss schemes.
  • +1/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Redundancy
    The logic to prepend 'redis://' to the URI if it does not match the pattern is duplicated in both 'redis-dump' and 'redis-load'. Consider abstracting this logic into a shared method to avoid code duplication and facilitate easier maintenance.

    Code Redundancy
    Similar to the issue in 'redis-dump', the logic to prepend 'redis://' to the URI if it does not match the pattern is duplicated here. Abstracting this logic into a shared method would improve code maintainability.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Modify the URI prefix logic to correctly handle both 'redis' and 'rediss' protocols

    To support both 'redis' and 'rediss' (Redis over SSL) protocols, ensure that the URI
    prefix is correctly set based on the existing protocol in obj.global.uri. Instead of
    always prepending 'redis://', check if 'rediss://' is required.

    exe/redis-dump [59]

    -obj.global.uri = 'redis://' << obj.global.uri unless obj.global.uri.match(/^rediss?:\/\//)
    +unless obj.global.uri.match(/^rediss?:\/\//)
    +  obj.global.uri = obj.global.uri.start_with?('rediss://') ? 'rediss://' << obj.global.uri : 'redis://' << obj.global.uri
    +end
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly addresses the need to handle both 'redis' and 'rediss' protocols, ensuring the URI is properly formatted for secure and non-secure connections. This is a significant improvement for robustness.

    9
    Ensure correct URI formatting for both 'redis' and 'rediss' protocols

    To avoid potential issues with URI manipulation, refactor the code to prepend the
    correct protocol ('redis://' or 'rediss://') based on the existing URI. This ensures
    that the URI is correctly formatted for both secure and non-secure Redis
    connections.

    exe/redis-load [54]

    -obj.global.uri = 'redis://' << obj.global.uri unless obj.global.uri.match(/^rediss?:\/\//)
    +unless obj.global.uri.match(/^rediss?:\/\//)
    +  obj.global.uri = obj.global.uri.start_with?('rediss://') ? 'rediss://' << obj.global.uri : 'redis://' << obj.global.uri
    +end
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion ensures that the URI is correctly formatted for both secure and non-secure Redis connections, which is crucial for avoiding potential issues with URI manipulation. This enhances the reliability of the code.

    9

    @joey-squid
    Copy link
    Author

    I'm not sure if the commit signing needs to be done by me or a maintainer. I don't have it set up and I'm on my way out the door on leave, so if I need to do anything I'm sorry but it won't happen until September. (Feel free to close this PR, make your own, whatever.)

    @managedhostingsolution
    Copy link

    managedhostingsolution commented Oct 2, 2024

    simple patch
    sed -i 's/redis:/rediss?:/g' "/var/lib/gems/3.1.0/gems/redis-dump-0.6.1/exe/redis-dump" and sed -i 's/redis:/rediss?:/g' "/var/lib/gems/3.1.0/gems/redis-dump-0.6.1/exe/redis-load"

    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