Skip to content

NIFI-8128: add support for specifying a password for Sentinel - #7481

Closed
EndzeitBegins wants to merge 5 commits into
apache:mainfrom
EndzeitBegins:NIFI-8128
Closed

NIFI-8128: add support for specifying a password for Sentinel#7481
EndzeitBegins wants to merge 5 commits into
apache:mainfrom
EndzeitBegins:NIFI-8128

Conversation

@EndzeitBegins

Copy link
Copy Markdown
Contributor

Summary

NIFI-8128

In order to reproduce the issue in a test case the Redis Sentinel setting sentinel auth-pass is required, which is only available in a newer Redis server binary as the one bundled into the com.github.kstyrc:embedded-redis dependency.
Thus I bundled a more recent binary (for Linux only).

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 17

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

@exceptionfactory exceptionfactory left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution @EndzeitBegins.

Including the Redis Server binary in the source repository is not a good strategy for testing due to size and version considerations. One option could be to evaluate an integration test using Testcontainers, but in absence of something else, it would be best to remove the server binary and associated integration test methods.

@EndzeitBegins

Copy link
Copy Markdown
Contributor Author

@exceptionfactory Thanks for the quick review.

I understand. I was thinking the same, but assumed it might be okay-ish due to the fact, that the test library used embedded-redis does the same.

I'll take a look at replacing embedded-redis, which hasn't seen maintenance for a long time by the way, with Testcontainers.

If that proves to time consuming I'd propose to remove the binary and @Disable the one test that requires a newer version completely, with an hint that in order to execute it, one must provide a newer binary locally.

I think it's a good thing to at least have a test case that reproduces an undesired behaviour, before fixing it.

Would you be fine with both approaches?

@EndzeitBegins

Copy link
Copy Markdown
Contributor Author

I removed both the binary file and the dependency on the unmaintained embedded-redis test library.

It proved a little more complicated than anticipated to get the tests working with Testcontainers, due to the auto-discovery mechanisms of Redis (Sentinel), but it seems to work now.

I would appreciate if you'd take another look at the PR, @exceptionfactory.

@exceptionfactory exceptionfactory left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for making the changes @EndzeitBegins! Using Testcontainers look much better than the previous approach. I noted a couple very minor recommendations, but this looks like a good way to go.

Comment thread nifi-nar-bundles/nifi-redis-bundle/nifi-redis-extensions/pom.xml Outdated
EndzeitBegins and others added 2 commits July 15, 2023 17:34
Co-authored-by: exceptionfactory <exceptionfactory@apache.org>
@EndzeitBegins

Copy link
Copy Markdown
Contributor Author

I applied the recommended changes, @exceptionfactory. Thank you for the code review.

@exceptionfactory exceptionfactory left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for working through the feedback @EndzeitBegins, the latest changes look good and the updated integration tests with Testcontainers work as expected. +1 merging

exceptionfactory pushed a commit that referenced this pull request Jul 16, 2023
- Replaced embedded-redis and with Testcontainers for Redis integration tests

This closes #7481

Signed-off-by: David Handermann <exceptionfactory@apache.org>

(cherry picked from commit 34a6d8a)
@EndzeitBegins
EndzeitBegins deleted the NIFI-8128 branch July 16, 2023 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants