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.

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

fix: make sure api server informer does not stop after setting change #9842

Merged
merged 1 commit into from Jun 30, 2022

Conversation

alexmt
Copy link
Collaborator

@alexmt alexmt commented Jun 30, 2022

Signed-off-by: Alexander Matyushentsev AMatyushentsev@gmail.com

PR fixes the bug introduced by #9778. After changing any SSO-related setting the API server returns a stale list of applications/projects. The issue is happening because API server attempts to restart application/app project informers which is not supported. Informer just stops and quietly refuses to start.

@alexmt alexmt added the cherry-pick/2.4 Candidate for cherry picking into the 2.4 release branch label Jun 30, 2022
@alexmt alexmt requested a review from jessesuen June 30, 2022 21:15
Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
@@ -160,6 +160,7 @@ func NewCommand() *cobra.Command {
stats.StartStatsTicker(10 * time.Minute)
stats.RegisterHeapDumper("memprofile")
argocd := server.NewServer(context.Background(), argoCDOpts)
argocd.Init(context.Background())
Copy link
Member

Choose a reason for hiding this comment

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

This is not really your problem, but just noticed we call context.Background() three times in same function and you might consider consolidating to one ctx.

Copy link
Contributor

Choose a reason for hiding this comment

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

This problem is fairly pervasive throughout cmd/.

I may look at this a little bit since it's an easy opportunity to get to know the codebase more.

@codecov
Copy link

codecov bot commented Jun 30, 2022

Codecov Report

Merging #9842 (8d732d3) into master (71c1f54) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #9842   +/-   ##
=======================================
  Coverage   45.86%   45.86%           
=======================================
  Files         227      227           
  Lines       26862    26862           
=======================================
  Hits        12320    12320           
  Misses      12862    12862           
  Partials     1680     1680           
Impacted Files Coverage Δ
server/server.go 53.18% <100.00%> (ø)

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 71c1f54...8d732d3. Read the comment docs.

@alexmt alexmt enabled auto-merge (squash) June 30, 2022 21:58
@alexmt alexmt merged commit ee7356a into argoproj:master Jun 30, 2022
alexmt added a commit that referenced this pull request Jun 30, 2022
…#9842)

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
@alexmt alexmt deleted the api-restart-informers branch June 30, 2022 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/2.4 Candidate for cherry picking into the 2.4 release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants