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

[ACM-8232] call addonMgr.start asynchronously #1263

Closed
wants to merge 1 commit into from
Closed

[ACM-8232] call addonMgr.start asynchronously #1263

wants to merge 1 commit into from

Conversation

subbarao-meduri
Copy link
Collaborator

A workaround suggested by @elgnay is to call addonMgr.start() asynchronously.
Together with fix stolostron/multiclusterhub-operator#1169 should resolve the issue.

I have verified that this works in my local env.

@subbarao-meduri
async addonMgr.start
b1c1561 
Signed-off-by: Subbarao Meduri <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

saswatamcode
saswatamcode reviewed Oct 19, 2023
View reviewed changes
Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

/lgtm as far as I understand this.

operators/multiclusterobservability/pkg/certificates/cert_controller.go
os.Exit(1)
}
go func() {
ctx := context.TODO()
Copy link
Member

Choose a reason for hiding this comment

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

Probably needs some centralized context, in the future

@saswatamcode
Copy link
Member

The lint check is fixed by #1262

@saswatamcode
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 19, 2023
@openshift-ci
Copy link

openshift-ci bot commented Oct 19, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saswatamcode, subbarao-meduri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@saswatamcode
Copy link
Member

/retest

@douglascamata
Copy link
Contributor

The goconst linter's failing again with the "true" string. Maybe we should disable this linter? (cc @saswatamcode)

@saswatamcode
Copy link
Member

The goconst linter's failing again with the "true" string. Maybe we should disable this linter? (cc @saswatamcode)

Fixed in #1262! Just needs to be rebased for check to pass.
Fwiw, all of these "true" string checks, actually just check if a env var is set or not. We can refactor them out and remove that string

@elgnay
Copy link

elgnay commented Oct 19, 2023

@subbarao-meduri I think we no longer need this change. stolostron/multiclusterhub-operator#1169 is enough to fix the issue.

@douglascamata
Copy link
Contributor

If we indeed don't need this, as @elgnay says, I prefer to avoid the change. 😬

@subbarao-meduri
Copy link
Collaborator Author

Closing this PR, as confirmed not needed.

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

@saswatamcode saswatamcode saswatamcode left review comments

@douglascamata douglascamata Awaiting requested review from douglascamata

Assignees

@saswatamcode saswatamcode

Labels
approved dco-signoff: yes do-not-merge/hold lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@subbarao-meduri @saswatamcode @douglascamata @elgnay