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

feat: add redis #220

Merged
merged 5 commits into from
Sep 12, 2024
Merged

feat: add redis #220

merged 5 commits into from
Sep 12, 2024

Conversation

ambersun1234
Copy link
Contributor

@ambersun1234 ambersun1234 commented Sep 11, 2024

PR Type

Enhancement, Configuration changes


Description

  • Added a new helper function redis-test.image to return the proper Redis test image name.
  • Updated .helmignore to exclude temporary charts.
  • Added Redis as a dependency in the Helm chart.
  • Created a new template for Redis secret configuration.
  • Updated Captain deployment to include Redis configuration, including an init container and environment variables.
  • Added Redis configuration parameters to the values file.

Changes walkthrough 📝

Relevant files
Enhancement
_helpers.tpl
Add helper function for Redis test image                                 

charts/agh3/templates/_helpers.tpl

  • Added a new helper function redis-test.image to return the proper
    Redis test image name.
  • +7/-0     
    Chart.yaml
    Add Redis dependency to Helm chart                                             

    charts/agh3/Chart.yaml

    • Added Redis as a dependency in the Helm chart.
    +4/-0     
    redis-secret.yml
    Add Redis secret template                                                               

    charts/agh3/templates/base/redis-secret.yml

    • Created a new template for Redis secret configuration.
    +24/-0   
    captain-deployment.yml
    Update Captain deployment to include Redis configuration 

    charts/agh3/templates/captain/captain-deployment.yml

  • Added an init container for Redis in the Captain deployment.
  • Included Redis environment variables in the Captain deployment.
  • +27/-0   
    values.yaml
    Add Redis configuration parameters                                             

    charts/agh3/values.yaml

    • Added Redis configuration parameters to the values file.
    +34/-0   
    Configuration changes
    .helmignore
    Update .helmignore to exclude temporary charts                     

    .helmignore

    • Added tmpcharts* to the .helmignore file.
    +1/-0     

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


    @ambersun1234 ambersun1234 self-assigned this Sep 11, 2024
    @ai-themis
    Copy link

    ai-themis bot commented Sep 11, 2024

    PR Reviewer Guide 🔍

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

    Code Smell
    The helper function redis-test.image does not handle the case where .Values.redis.helpers.test.image might be undefined or null. Consider adding a default value or a check to handle such cases.

    Possible Bug
    The init container for Redis waits for the Redis service using a hardcoded service name redis-master. This might not work if the service name is different. Consider making the service name configurable.

    @ai-themis
    Copy link

    ai-themis bot commented Sep 11, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Simplify the Redis readiness check by using redis-cli's built-in retry mechanism

    The command field for the captain-init-redis init container uses a shell script to
    wait for Redis to be ready. This can be simplified and made more robust by using the
    redis-cli's built-in retry mechanism.

    charts/agh3/templates/captain/captain-deployment.yml [31-35]

     command:
       [
    -    "sh",
    -    "-c",
    -    "until redis-cli -h redis-master.$(NAMESPACE).svc.cluster.local -a $REDIS_PASSWORD ping; do echo waiting for redis; sleep 1; done",
    +    "redis-cli",
    +    "-h",
    +    "redis-master.$(NAMESPACE).svc.cluster.local",
    +    "-a",
    +    "$REDIS_PASSWORD",
    +    "--retry",
    +    "30",
    +    "ping"
       ]
     
    Suggestion importance[1-10]: 9

    Why: This suggestion simplifies the readiness check and makes it more robust by leveraging redis-cli's built-in retry mechanism, which is a significant improvement.

    9
    Possible issue
    Use base64 encoding for the password field in Kubernetes Secrets to handle special characters and newlines properly

    The stringData field in Kubernetes Secrets is intended for storing unencoded string
    data. However, the password field is being populated with a potentially multiline
    string. This could lead to issues if the password contains special characters or
    newlines. Consider using data with base64-encoded values instead.

    charts/agh3/templates/base/redis-secret.yml [9-23]

    -stringData:
    +data:
       password: {{
           default 
    -        .Values.redis.secret.password
    +        (b64enc .Values.redis.secret.password)
             (
               include "specify-password"
                 (
                   dict
                     "domain" (default .Values.ingress.host "app.argushack.com")
                     "token" .Values.keygen.apiToken
                     "prefix" .Values.redis.secret.secretName
                 )
             )
    -    | quote
    +    | b64enc
       }}
     
    Suggestion importance[1-10]: 8

    Why: This suggestion addresses a potential issue with special characters and newlines in the password field, improving the robustness of the secret handling.

    8
    Best practice
    Set the redis.enabled parameter to false by default to prevent unintentional Redis deployments

    The redis.enabled parameter is set to true by default. This might not be desirable
    for all environments. Consider setting it to false by default to avoid unintentional
    Redis deployments.

    charts/agh3/values.yaml [345]

    -enabled: true
    +enabled: false
     
    Suggestion importance[1-10]: 7

    Why: Setting redis.enabled to false by default is a good practice to avoid unintentional deployments, though it might not be crucial for all environments.

    7
    Maintainability
    Expand the .helmignore file to ignore more common temporary and build files

    The .helmignore file currently only ignores tmpcharts. Consider adding more common
    temporary and build files to the ignore list, such as .log, .tmp, and *.bak.
    *

    .helmignore [1]

     tmpcharts*
    +*.log
    +*.tmp
    +*.bak
     
    Suggestion importance[1-10]: 6

    Why: Adding more common temporary and build files to the .helmignore file improves maintainability, but it is a minor enhancement.

    6

    @Aries0d0f Aries0d0f merged commit 8b990d1 into main Sep 12, 2024
    1 check passed
    @Aries0d0f Aries0d0f deleted the feat/redis branch September 12, 2024 04:44
    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