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

Fixing bug with new Avro conversion when message values are NULL #3388

Merged
merged 7 commits into from
Mar 30, 2024

Conversation

passuied
Copy link
Contributor

@passuied passuied commented Mar 29, 2024

Description

Fixing bug with new Avro conversion when message values are NULL

Issue reference

#3387

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation - N/A

@passuied passuied requested review from a team as code owners March 29, 2024 19:00
Signed-off-by: Patrick Assuied <[email protected]>
…bout it not being wrapped in a CloudEvent

Signed-off-by: Patrick Assuied <[email protected]>
@passuied passuied force-pushed the bug/fix-kafka-pubsub-avro-null-value branch from 56842d2 to eff81be Compare March 29, 2024 21:04
@passuied
Copy link
Contributor Author

@yaron2 These certification tests are really brittle. I doubt my changes have caused rabbitmq and mqtt3 tests to fail... Any path forward other than retrying these tests over an over? Should they be skipped until made more resilient?

@passuied
Copy link
Contributor Author

passuied commented Mar 29, 2024

The current issue in cert tests seems realted to a certificate issue:

failed to verify certificate: x509: certificate has expired or is not yet valid

I remember this was an issue previously due to some certificates being hardcoded in tests and expiring...

Yeah these hard-coded certs have expired... https://github.com/dapr/components-contrib/blob/main/tests/certification/bindings/rabbitmq/mtls_sasl_external/components/mtls_external/rabbitmq-mtls.yaml

Copy link
Contributor

@ItalyPaleAle ItalyPaleAle left a comment

Choose a reason for hiding this comment

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

Small changes

common/component/kafka/kafka.go Outdated Show resolved Hide resolved
common/component/kafka/kafka.go Outdated Show resolved Hide resolved
common/component/kafka/kafka_test.go Outdated Show resolved Hide resolved
passuied and others added 2 commits March 29, 2024 17:07
Co-authored-by: Alessandro (Ale) Segala <[email protected]>
Signed-off-by: Patrick Assuied <[email protected]>
Co-authored-by: Alessandro (Ale) Segala <[email protected]>
Signed-off-by: Patrick Assuied <[email protected]>
@passuied passuied requested a review from ItalyPaleAle March 30, 2024 00:08
@passuied
Copy link
Contributor Author

passuied commented Mar 30, 2024

The current issue in cert tests seems realted to a certificate issue:

failed to verify certificate: x509: certificate has expired or is not yet valid

I remember this was an issue previously due to some certificates being hardcoded in tests and expiring...

Yeah these hard-coded certs have expired... https://github.com/dapr/components-contrib/blob/main/tests/certification/bindings/rabbitmq/mtls_sasl_external/components/mtls_external/rabbitmq-mtls.yaml

@ItalyPaleAle how should we deal with these certification errors due to certs having expired. They've been failing since Feb... In the past, you figured out a way to generate certs as part of the test (though I cannot find the code anymore)...

@ItalyPaleAle
Copy link
Contributor

@ItalyPaleAle how should we deal with these certification errors due to certs having expired. They've been failing since Feb... In the past, you figured out a way to generate certs as part of the test (though I cannot find the code anymore)...

That's for RabbitMQ not Kafka, right? If you want to contribute a PR to fix that, it'd be nice :)

I think accepting hardcoded certs was a mistake. We should have generated the certs on-the-fly.

Co-authored-by: Alessandro (Ale) Segala <[email protected]>
Signed-off-by: Patrick Assuied <[email protected]>
@passuied
Copy link
Contributor Author

passuied commented Mar 30, 2024

@ItalyPaleAle how should we deal with these certification errors due to certs having expired. They've been failing since Feb... In the past, you figured out a way to generate certs as part of the test (though I cannot find the code anymore)...

That's for RabbitMQ not Kafka, right? If you want to contribute a PR to fix that, it'd be nice :)

I think accepting hardcoded certs was a mistake. We should have generated the certs on-the-fly.

Don't really have the time right this minute but would like my PR to go through quickly as we're eager to use the feature. I can try to contribute later but I'm struggling to run these tests locally...

@ItalyPaleAle
Copy link
Contributor

I am not concerned with the RabbitMQ tests failing in this PR. If Kafka tests pass, we can merge

ItalyPaleAle
ItalyPaleAle previously approved these changes Mar 30, 2024
Signed-off-by: Patrick Assuied <[email protected]>
@yaron2 yaron2 merged commit cfee998 into dapr:main Mar 30, 2024
86 of 89 checks passed
passuied added a commit to passuied/components-contrib that referenced this pull request Mar 30, 2024
…r#3388)

Signed-off-by: Patrick Assuied <[email protected]>
Co-authored-by: Alessandro (Ale) Segala <[email protected]>
ItalyPaleAle added a commit that referenced this pull request Apr 1, 2024
Signed-off-by: Patrick Assuied <[email protected]>
Co-authored-by: Alessandro (Ale) Segala <[email protected]>
@berndverst berndverst added this to the v1.14 milestone Jul 23, 2024
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.

4 participants