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

#295 Fix NPE for listing plugins when connector class not found #299

Merged
merged 1 commit into from
Dec 27, 2022

Conversation

C0urante
Copy link
Contributor

Addresses #295

@C0urante
Copy link
Contributor Author

CC @gunnarmorling 😃

int exitCode = context.commandLine().execute("-f", path.toAbsolutePath().toString());

assertThat(exitCode).isEqualTo(CommandLine.ExitCode.SOFTWARE);
assertThat(context.output().toString()).contains("Specified class isn't a valid connector type");
Copy link
Contributor Author

@C0urante C0urante Dec 27, 2022

Choose a reason for hiding this comment

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

I wanted to add an assertion about it actually showing the list of available connectors, but was having some trouble running the integration tests locally (containers were hanging on startup).

While less powerful, this test still guards against the NPE addressed by this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that container issue on Mac M1? If so, I've recently pushed an update which seems to help for the Kafka image, but Connect still seems slow. It's not quite clear to my why, as the Debezium 2.0 images have an Arm flaviour. Perhaps Testcontainers is not pulling that in for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indeed on M1!

I've taken another stab at this on the latest main and am no longer seeing these issues. I thought I'd pulled the latest main before creating the branch for this PR, but maybe not?

Either way, filed #302 to add this missing test coverage and (of course 🙃) fix a bug that it unearthed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is indeed on M1!

I've taken another stab at this on the latest main and am no longer seeing these issues.

Happy to hear! One thing we could do to further speed things up a bit is to use the debezium-base image (i.e. empty KC) rather than debezium, as scanning the plug-in path definitely takes some time. That'd require to adjust a few tests though, e.g. on expected connector plug-ins.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Logged #303 for this.

@gunnarmorling gunnarmorling merged commit 8ba41dd into kcctl:main Dec 27, 2022
@gunnarmorling
Copy link
Collaborator

Applied; thank you so much, @C0urante!

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

2 participants