Skip to content
This repository was archived by the owner on Dec 13, 2023. It is now read-only.

Conversation

@davidwadden
Copy link
Contributor

This updates Conductor to use changes in the dyno library to connect to Redis instances requiring password authentication. It does so by parsing an optional 4th colon-delimited string from workflow.dynomite.cluster.hosts to set the password on the underlying Jedis client. (shown below)

I'm looking for any feedback to support getting this change merged.


See Netflix/dyno#244 for the underlying changes, included as transitive dependency with dyno-queues:2.0.0-rc5 upgrade.

https://github.com/Netflix/dyno/blob/v1.6.5/dyno-jedis/src/main/java/com/netflix/dyno/jedis/JedisConnectionFactory.java#L80-L93

@codecov
Copy link

codecov bot commented Feb 5, 2019

Codecov Report

Merging #972 into dev will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #972      +/-   ##
============================================
+ Coverage     62.07%   62.22%   +0.14%     
- Complexity     2548     2556       +8     
============================================
  Files           225      225              
  Lines         12885    12888       +3     
  Branches       1280     1281       +1     
============================================
+ Hits           7999     8019      +20     
+ Misses         4177     4159      -18     
- Partials        709      710       +1
Impacted Files Coverage Δ Complexity Δ
...uctor/jedis/ConfigurationHostSupplierProvider.java 82.6% <100%> (+82.6%) 7 <0> (+7) ⬆️
.../netflix/conductor/dyno/DynomiteConfiguration.java 57.14% <0%> (+7.14%) 5% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7eecae3...a24b7c8. Read the comment docs.

@coveralls
Copy link

coveralls commented Feb 5, 2019

Pull Request Test Coverage Report for Build 2347

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 68.233%

Totals Coverage Status
Change from base Build 2345: 0.1%
Covered Lines: 9002
Relevant Lines: 13193

💛 - Coveralls

@kishorebanala
Copy link
Contributor

@davidwadden Looks good. Would you mind fixing the codecov checks please, so that I can go ahead and merge. Current set-up doesn't allow me to merge.

@apanicker-nflx
Copy link
Collaborator

@davidwadden Thanks for your contribution.
Please also update the ReadMe at https://github.com/Netflix/conductor/tree/master/server with an example.

  + Upgrades dyno-queues to v2.0.0-rc5

Signed-off-by: Carlton Schuyler <[email protected]>
@kishorebanala kishorebanala merged commit 787f856 into Netflix:dev Feb 7, 2019
@davidwadden davidwadden deleted the redis-auth branch February 9, 2019 06:08
long-64 pushed a commit to long-64/conductor that referenced this pull request Oct 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants