Skip to content

Conversation

@crenshaw-dev
Copy link
Member

This lint rule seems mostly very good. It found a bunch of unnecessary code.

The only false positives were two structs that have fields that are being used to make unique map keys.

@crenshaw-dev crenshaw-dev requested review from a team as code owners June 3, 2025 16:01
@bunnyshell
Copy link

bunnyshell bot commented Jun 3, 2025

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

Signed-off-by: Michael Crenshaw <[email protected]>
Comment on lines -159 to -160
# Cache expiration for failed login attempts (default 24h0m0s)
server.login.attempts.expiration: "24h0m0s"
Copy link
Member Author

Choose a reason for hiding this comment

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

Setting this was actually a no-op. Need to investigate more. Maybe we switched from an in-memory cache to a Redis cache and just didn't update the code?

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR replaced the redis-based failures counter with an in-memory solution that doesn't have a configurable expiration: #5477

Not going to reevaluate that decision for this PR, just going to remove the dead code.

Signed-off-by: Michael Crenshaw <[email protected]>
Signed-off-by: Michael Crenshaw <[email protected]>
@codecov
Copy link

codecov bot commented Jun 3, 2025

Codecov Report

❌ Patch coverage is 92.85714% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.99%. Comparing base (ea97dec) to head (5da1d17).
⚠️ Report is 421 commits behind head on master.

Files with missing lines Patch % Lines
...plicationset/services/scm_provider/azure_devops.go 0.00% 1 Missing ⚠️
cmd/argocd/commands/admin/app.go 0.00% 1 Missing ⚠️
cmd/argocd/commands/headless/headless.go 0.00% 1 Missing ⚠️
server/application/terminal.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #23242      +/-   ##
==========================================
- Coverage   60.02%   59.99%   -0.03%     
==========================================
  Files         342      341       -1     
  Lines       57870    57818      -52     
==========================================
- Hits        34736    34689      -47     
+ Misses      20352    20346       -6     
- Partials     2782     2783       +1     

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

Comment on lines -1280 to -1281
mgr.reposCache = nil
mgr.repoCredsCache = nil
Copy link
Member Author

Choose a reason for hiding this comment

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

These fields aren't read, so this method doesn't do anything.

Copy link
Member

@ranakan19 ranakan19 left a comment

Choose a reason for hiding this comment

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

Looks Good, Thanks for the changes and leaving some comments on the reviews, they were helpful.

Quite a cleanup of the ArgoCDRepoServer struct! 👏🏽

@crenshaw-dev
Copy link
Member Author

@ranakan19 thanks! Yeah the cleanup on ArgoCDRepoServer made me very happy :-D

Copy link
Member

@ishitasequeira ishitasequeira left a comment

Choose a reason for hiding this comment

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

LGTM!! Thanks for the amazing cleanup @crenshaw-dev !! 🙌 🚀

@ishitasequeira ishitasequeira merged commit 30a0088 into argoproj:master Jun 5, 2025
28 checks passed
dsuhinin pushed a commit to dsuhinin/argo-cd that referenced this pull request Jun 16, 2025
dsuhinin pushed a commit to dsuhinin/argo-cd that referenced this pull request Jun 16, 2025
enneitex pushed a commit to enneitex/argo-cd that referenced this pull request Aug 24, 2025
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.

3 participants