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 GRPC CheckRegistryServer function #2756

Merged
merged 1 commit into from
Apr 29, 2022
Merged

Fix GRPC CheckRegistryServer function #2756

merged 1 commit into from
Apr 29, 2022

Conversation

awgreene
Copy link
Member

Problem: The CheckRegistryServer function used by grpc catalogSources
does not confirm that the serviceAccount associated with the
catalogSource exists.

Solution: Update the GRPC CheckRegistryServer function to check if the
serviceAccount exists.

Signed-off-by: Alexander Greene [email protected]

@openshift-ci openshift-ci bot requested review from akihikokuroda and anik120 April 28, 2022 17:04
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2022
@exdx
Copy link
Member

exdx commented Apr 28, 2022

/lgtm

Looks like there is a unit test failure -- may be related?

 --- FAIL: TestSyncCatalogSources (1.17s)
    --- FAIL: TestSyncCatalogSources/GRPCConnectionStateAddressIsUpdated (0.12s)

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2022
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@akihikokuroda
Copy link
Member

LGTM. Waiting for the unit test fix.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

timflannagan
timflannagan approved these changes Apr 28, 2022
View reviewed changes
Copy link
Member

@timflannagan timflannagan left a comment

Choose a reason for hiding this comment

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

LGTM - holding so the bot doesn't go crazy while we fix up those unit tests, and rebase this PR against the master branch to make branch protection happy.

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 28, 2022
@openshift-ci
Copy link

openshift-ci bot commented Apr 28, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, timflannagan

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:
  • OWNERS [awgreene,timflannagan]

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

@awgreene awgreene force-pushed the fix-grpc-check-registry-server branch from be86199 to e765729 Compare April 28, 2022 20:16
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2022
@awgreene
Copy link
Member Author

Looks like there is a unit test failure -- may be related?

@exdx seems like that unit test now needs a serviceAccount to be created in order to work as expected.

@exdx
Copy link
Member

exdx commented Apr 28, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2022
@awgreene awgreene force-pushed the fix-grpc-check-registry-server branch from e765729 to f87f09d Compare April 29, 2022 12:57
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2022
@timflannagan
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2022
@awgreene
Copy link
Member Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2022
@awgreene
Fix GRPC CheckRegistryServer function
da6cf22 
Problem: The CheckRegistryServer function used by grpc catalogSources
does not confirm that the serviceAccount associated with the
catalogSource exists.

Solution: Update the GRPC CheckRegistryServer function to check if the
serviceAccount exists.

Signed-off-by: Alexander Greene <[email protected]>
@awgreene awgreene force-pushed the fix-grpc-check-registry-server branch from f87f09d to da6cf22 Compare April 29, 2022 13:47
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2022
@timflannagan
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2022
@openshift-merge-robot openshift-merge-robot merged commit 7e8e9f7 into operator-framework:master Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timflannagan timflannagan timflannagan approved these changes

@akihikokuroda akihikokuroda Awaiting requested review from akihikokuroda

@anik120 anik120 Awaiting requested review from anik120

Assignees

@timflannagan timflannagan

@exdx exdx

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@awgreene @exdx @openshift-bot @akihikokuroda @timflannagan @openshift-merge-robot