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

CORE-20760: Add wait argument to member onboarding in GetRegistrationsTest #6278

Merged
merged 7 commits into from
Jul 19, 2024

Conversation

malachyb
Copy link
Contributor

@malachyb malachyb commented Jul 11, 2024

When onboarding members in GetRegistrationsTest, we have not been passing the --wait argument which has caused errors due to registration not being completed before the member is used. This PR reverts the temporary fix to the tests and adds the argument to prevent the issue from re-occurring.

I have also done a few test runs of the plugins smoke tests against this PR and the issue hasn't occurred in any of them.

@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Jul 11, 2024

Jenkins build for PR 6278 build 9

Build Successful:
Jar artifact version produced by this PR: 5.3.0.0-alpha-1721380583919
Helm chart version produced by this PR: 5.3.0-alpha.1721380583919
Helm chart pushed to: oci://corda-os-docker-dev.software.r3.com/helm-charts/pr-6278/corda
Helm chart Polaris score: 82

@malachyb malachyb marked this pull request as ready for review July 11, 2024 14:30
@malachyb malachyb requested a review from a team as a code owner July 11, 2024 14:30
@@ -86,6 +87,7 @@ class GetRegistrationsTest {
user,
password,
INSECURE,
WAIT
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add this to the registration on line 54?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Must have missed that one, thanks.

@@ -180,8 +179,6 @@ abstract class BaseOnboard : Runnable, RestCommand() {

if (hasKeys) return

println("Creating TLS key.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you taking these out?
They were put in to help spot where the journey breaks down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies, I must have misunderstood your comment on the original ticket. Could you point me to where the sleep is that you mentioned? I can't see any sleeps in the commit that I reverted.

Copy link
Contributor

Choose a reason for hiding this comment

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

The sleep was swapped for polling an alternate endpoint: #6247 (comment)

This is the code it was replaced with: 40164d5

I expect the println statements to remain, only the above executeWithRetry block to be taken out.

@@ -206,6 +197,9 @@ class OnboardMember : Runnable, BaseOnboard() {

setupNetwork()

println("Provided registration context: ")
println(memberRegistrationRequest)
Copy link
Contributor

Choose a reason for hiding this comment

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

Take these out

@malachyb malachyb requested a review from a team as a code owner July 16, 2024 15:06
@malachyb malachyb marked this pull request as draft July 16, 2024 15:07
Copy link

sonarcloud bot commented Jul 19, 2024

@malachyb malachyb marked this pull request as ready for review July 19, 2024 10:20
@malachyb malachyb removed the request for review from a team July 19, 2024 10:23
@malachyb malachyb merged commit fd1f51d into release/os/5.3 Jul 19, 2024
5 checks passed
@malachyb malachyb deleted the Malachy/CORE-20760 branch July 19, 2024 10:29
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.

None yet

3 participants