Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(Redis Node): Add support for username auth #12274

Merged
merged 6 commits into from
Dec 19, 2024

Conversation

netroy
Copy link
Member

@netroy netroy commented Dec 17, 2024

Summary

This PR adds support for username auth on Redis and RedisTrigger nodes.

To test this, you can use the following docker-compose snippet to start a redis container to test these nodes with:

services:
  redis-auth:
    image: redis/redis-stack
    container_name: redis-auth
    restart: unless-stopped
    environment:
      REDIS_ARGS: "--requirepass password --user username on >password ~* allcommands --user default off nopass nocommands"
    ports:
      - "8001:8001"
      - "6379:6379"

Related Linear tickets, Github issues, and Community forum posts

NODE-2174

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team node/improvement New feature or request labels Dec 17, 2024
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

Copy link
Contributor

@dana-gill dana-gill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, this looks good 😄 It looks like CodeCoverage is complaining about not being able to test this line. Could you add a utils.redisConnectionTest test? I think checking that setupRedisClient is called with the right credentials should suffice.

@dana-gill
Copy link
Contributor

Just to note as well: I tested this a bit differently from the PR description. Here is what I did:

  • Logged into my cloud account of Redis Stack
  • Created a new credential on n8n using the connection string from Redis Stack. I didn't explicitly create a user on Redis Stack so my username (which Redis automatically assigned to me) was default
  • Successfully tested the Info node using the new credential

My previous credentials also worked without having to change anything ✅

@netroy netroy requested a review from dana-gill December 18, 2024 13:25
Copy link
Contributor

@dana-gill dana-gill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of moving the tests to a __tests__ folder? I feel like it introduces a slightly new way to structure the tests

Copy link
Contributor

@dana-gill dana-gill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__tests__ make sense :)

Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

Copy link

cypress bot commented Dec 18, 2024

n8n    Run #8413

Run Properties:  status check passed Passed #8413  •  git commit d069fd9320: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 netroy 🗃️ e2e/*
Project n8n
Branch Review NODE-2174-add-redis-user-auth
Run status status check passed Passed #8413
Run duration 04m 43s
Commit git commit d069fd9320: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 netroy 🗃️ e2e/*
Committer कारतोफ्फेलस्क्रिप्ट™
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 480
View all changes introduced in this branch ↗︎

Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

1 similar comment
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@netroy netroy requested a review from dana-gill December 19, 2024 09:29
Copy link
Contributor

✅ All Cypress E2E specs passed

@netroy netroy merged commit 64c0414 into master Dec 19, 2024
37 checks passed
@netroy netroy deleted the NODE-2174-add-redis-user-auth branch December 19, 2024 09:58
@github-actions github-actions bot mentioned this pull request Dec 19, 2024
@janober
Copy link
Member

janober commented Dec 19, 2024

Got released with n8n@1.73.0

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
n8n team Authored by the n8n team node/improvement New feature or request Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants