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.

Sign up for GitHub

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

Jump to bottom

fix(cli): Add Redis password to forwardCacheClient struct (#19035) #19599

Merged
merged 1 commit into from
Sep 17, 2024
Merged

fix(cli): Add Redis password to forwardCacheClient struct (#19035) #19599

merged 1 commit into from
Sep 17, 2024

Conversation

NetanelK
Copy link
Contributor

@NetanelK NetanelK commented Aug 20, 2024 •
edited by agaudreault
Loading

Fixes #19035
Fixes #18426
Fixes #18464

  • Added optional redisPassword field in the forwardCacheClient struct, for headless mode.
  • Redis password is being passed from redisOptions into appstateCache builder

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@NetanelK
fix: Add redis password to forwardCacheClient struct
75348eb 
Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>
@NetanelK NetanelK requested a review from a team as a code owner August 20, 2024 09:57
@bunnyshell Bunnyshell
Copy link

bunnyshell bot commented Aug 20, 2024 •
edited
Loading

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@bunnyshell Bunnyshell
Copy link

bunnyshell bot commented Aug 20, 2024 •
edited
Loading

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@codecov Codecov
Copy link

codecov bot commented Aug 20, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@de53d8e). Learn more about missing BASE report.
Report is 189 commits behind head on master.

Files with missing lines Patch % Lines
cmd/argocd/commands/headless/headless.go 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##             master   #19599   +/-   ##
=========================================
  Coverage          ?   55.84%           
=========================================
  Files             ?      316           
  Lines             ?    43739           
  Branches          ?        0           
=========================================
  Hits              ?    24427           
  Misses            ?    16758           
  Partials          ?     2554

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NetanelK NetanelK mentioned this pull request Aug 20, 2024
Closed
3 tasks
jannfis
jannfis reviewed Aug 21, 2024
View reviewed changes
Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

Can we somehow test this, so that we won't run into a regression later on?

todaywasawesome
todaywasawesome reviewed Aug 21, 2024
View reviewed changes
Copy link
Contributor

@todaywasawesome todaywasawesome left a comment

Choose a reason for hiding this comment

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

Agreed, with tests this is a slam dunk.

@NetanelK
Copy link
Contributor Author

NetanelK commented Aug 22, 2024 •
edited
Loading

How can I test the headless mode? I would be happy to have some help with it

@tooptoop4
Copy link

who gon merge dis

@purajit purajit mentioned this pull request Sep 11, 2024
Open
pasha-codefresh
pasha-codefresh approved these changes Sep 17, 2024
View reviewed changes
Copy link
Member

@pasha-codefresh pasha-codefresh left a comment

Choose a reason for hiding this comment

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

Fix is fine, tested it locally also, it solves the issue.
Checking if here an easy way to cover this issue with e2e

@pasha-codefresh
Copy link
Member

@jannfis @todaywasawesome i am going to merge this PR. Here no easy way to cover core mode with e2e tests today and same with unit. It definitely makes sense to do it, but it will requires pretty big effort to do it. I think we need to merge it, because this PR fix an issue and making things better.

@NetanelK thank you, and sorry for delay in review

@pasha-codefresh pasha-codefresh merged commit fcc186b into argoproj:master Sep 17, 2024
32 of 33 checks passed
@pasha-codefresh
Copy link
Member

/cherry-pick release-2.13

gcp-cherry-pick-bot bot pushed a commit that referenced this pull request Sep 18, 2024
@NetanelK
fix: Add redis password to forwardCacheClient struct (#19599)
0143e04 
Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>
@gcp-cherry-pick-bot gcp-cherry-pick-bot bot mentioned this pull request Sep 18, 2024
Merged
pasha-codefresh pushed a commit that referenced this pull request Sep 18, 2024
@gcp-cherry-pick-bot @NetanelK
fix: Add redis password to forwardCacheClient struct (#19599) (#19977)
425b408 
Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>
Co-authored-by: Netanel Kadosh <kadoshnetanel@gmail.com>
@NetanelK
Copy link
Contributor Author

@pasha-codefresh, will this be added to older releases as a fix?

@pasha-codefresh
Copy link
Member

/cherry-pick release-2.12

@pasha-codefresh
Copy link
Member

/cherry-pick release-2.11

@pasha-codefresh
Copy link
Member

/cherry-pick release-2.12

@purajit
Copy link

purajit commented Sep 23, 2024

@pasha-codefresh when should we expect the 2.11/12 releases to come through?

@pasha-codefresh
Copy link
Member

@purajit it requires manual cherry-picking, i will do it today, and release itself we can do for example next Monday

@garrmark
Copy link

garrmark commented Oct 2, 2024

did this happen @pasha-codefresh ?

NetanelK added a commit to NetanelK/argo-cd that referenced this pull request Oct 7, 2024
@NetanelK
fix: Add redis password to forwardCacheClient struct (argoproj#19599)
99024c5 
Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>
NetanelK added a commit to NetanelK/argo-cd that referenced this pull request Oct 7, 2024
@NetanelK
fix: Add redis password to forwardCacheClient struct (argoproj#19599)
b37163d 
Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>
@NetanelK
Copy link
Contributor Author

NetanelK commented Oct 7, 2024

@pasha-codefresh I've created cherrypick PRs (#20261, #20262) for this fix, can you take a look?

@nitishfy nitishfy mentioned this pull request Oct 7, 2024
Merged
14 tasks
NetanelK added a commit to NetanelK/argo-cd that referenced this pull request Oct 7, 2024
@NetanelK
fix: Add redis password to forwardCacheClient struct (argoproj#19599)
689513c 
Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>
NetanelK added a commit to NetanelK/argo-cd that referenced this pull request Oct 9, 2024
@NetanelK
fix: Add redis password to forwardCacheClient struct (argoproj#19599)
05241f8 
Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>
NetanelK added a commit to NetanelK/argo-cd that referenced this pull request Oct 9, 2024
@NetanelK
fix: Add redis password to forwardCacheClient struct (argoproj#19599)
2d1e339 
Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>
NetanelK added a commit to NetanelK/argo-cd that referenced this pull request Oct 9, 2024
@NetanelK
fix: Add redis password to forwardCacheClient struct (argoproj#19599)
9aef555 
Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>
NetanelK added a commit to NetanelK/argo-cd that referenced this pull request Oct 9, 2024
@NetanelK
fix: Add redis password to forwardCacheClient struct (argoproj#19599)
98e51c7 
Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>
NetanelK added a commit to NetanelK/argo-cd that referenced this pull request Oct 9, 2024
@NetanelK
fix: Add redis password to forwardCacheClient struct (argoproj#19599)
95de77a 
Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>
NetanelK added a commit to NetanelK/argo-cd that referenced this pull request Oct 9, 2024
@NetanelK
fix: Add redis password to forwardCacheClient struct (argoproj#19599)
91b6cd6 
Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>
@pasha-codefresh
Copy link
Member

Thank you @NetanelK , checking

pasha-codefresh pushed a commit that referenced this pull request Oct 9, 2024
@NetanelK @sheikhrachel @crenshaw-dev
fix(cli): cherrypick Redis password fix #19599 into 2.11 (#20261)
b9567a4 
* fix(cli): add optional password setting for headless redis client (#19035) (#19039)

* chore: add optional password setting for headless redis client

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>

* fix: remove import cycle

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>

* fix: add shared SetOptionalRedisPasswordFromKubeConfig method

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>

* fix: export redis consts

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>

* test: add test cases for SetOptionalRedisPasswordFromKubeConfig()

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>

* chore: go mod tidy

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>

* fix: use require instead of assert

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>

* fix: Update common/common.go

Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: Rachel Sheikh <sheikhrachel97@gmail.com>

---------

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>
Signed-off-by: Rachel Sheikh <sheikhrachel97@gmail.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* fix: Add redis password to `forwardCacheClient` struct (#19599)

Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>

---------

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>
Signed-off-by: Rachel Sheikh <sheikhrachel97@gmail.com>
Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>
Co-authored-by: Rachel Sheikh <sheikhrachel97@gmail.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
pasha-codefresh pushed a commit that referenced this pull request Oct 9, 2024
@NetanelK @sheikhrachel @crenshaw-dev
fix(cli): cherrypick Redis password fix #19599 into 2.12 (#20262)
3ff57d2 
* fix(cli): add optional password setting for headless redis client (#19035) (#19039)

* chore: add optional password setting for headless redis client

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>

* fix: remove import cycle

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>

* fix: add shared SetOptionalRedisPasswordFromKubeConfig method

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>

* fix: export redis consts

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>

* test: add test cases for SetOptionalRedisPasswordFromKubeConfig()

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>

* chore: go mod tidy

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>

* fix: use require instead of assert

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>

* fix: Update common/common.go

Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: Rachel Sheikh <sheikhrachel97@gmail.com>

---------

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>
Signed-off-by: Rachel Sheikh <sheikhrachel97@gmail.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* fix: Add redis password to `forwardCacheClient` struct (#19599)

Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>

---------

Signed-off-by: Rachel Sheikh <rsheikh@squareup.com>
Signed-off-by: Rachel Sheikh <sheikhrachel97@gmail.com>
Signed-off-by: Netanel Kadosh <kadoshnetanel@gmail.com>
Co-authored-by: Rachel Sheikh <sheikhrachel97@gmail.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
@purajit
Copy link

purajit commented Oct 10, 2024

Thanks! I see the cherry pick branches - when will a release happen?

@purajit
Copy link

purajit commented Oct 17, 2024

@pasha-codefresh any updates on the release?

@pasha-codefresh
Copy link
Member

I didnt plan it, better ask in contributions channel

@purajit
Copy link

purajit commented Oct 24, 2024

@pasha-codefresh Gotcha, I assumed the

@purajit it requires manual cherry-picking, i will do it today, and release itself we can do for example next Monday

message meant that a release would be cut.

Do you mean #argo-cd-contributors on the CNCF Slack?

Also how far is the 2.13 release? It's been a month of waiting on releases for this important
fix, so if there's a lot of process to go through to get it done, I can just wait for 2.13 if
it's coming soon.

@pasha-codefresh
Copy link
Member

2.13 GA Monday, Nov. 4, 2024.

@NetanelK
Copy link
Contributor Author

@purajit Actually, this was released with v2.11.10 and v2.12.5 releases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jannfis jannfis jannfis left review comments

@todaywasawesome todaywasawesome todaywasawesome left review comments

@pasha-codefresh pasha-codefresh pasha-codefresh approved these changes

@leoluz leoluz Awaiting requested review from leoluz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@NetanelK @tooptoop4 @pasha-codefresh @purajit @garrmark @todaywasawesome @jannfis