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

refactor(cloudevents-server): add kafka topic to store event #165

Merged
merged 1 commit into from
Sep 8, 2024

Conversation

wuhuizuo
Copy link
Contributor

@wuhuizuo wuhuizuo commented Aug 9, 2024

Signed-off-by: wuhuizuo wuhuizuo@126.com

Copy link

ti-chi-bot bot commented Aug 9, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

The pull request seems to refactor the cloudevents-server by adding a Kafka topic to store events.

Key changes include:

  1. go.mod and go.sum files have been updated to include new dependencies related to Kafka and other related packages.
  2. cloudevents-server/handlers.go and cloudevents-server/main.go have been modified to use new Kafka related event handlers.
  3. New files related to Kafka event handling have been added: cloudevents-server/pkg/events/handler/kafka.go, cloudevents-server/pkg/kafka/auth.go, cloudevents-server/pkg/kafka/consumer.go, cloudevents-server/pkg/kafka/producer.go, cloudevents-server/pkg/kafka/types.go
  4. The configuration (cloudevents-server/pkg/config/config.go) has been updated to include Kafka related configurations.

Potential problems:

  1. Lack of comments in the code making it hard to understand the purpose of functions.
  2. No tests have been updated or added in this pull request. If there were any tests related to the modified functions, they should be updated. New tests should also be written to cover the added code.

Suggestions:

  1. Add comments to the new functions and any complex or unclear lines of code.
  2. Update any existing tests that cover the modified functions, and add new tests for the new functionality.
  3. It would be helpful if the pull request description included more details about why these changes were made and what the expected impact is.

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@wuhuizuo wuhuizuo force-pushed the feature/cloudevent-producer-and-consumer branch from 1caa168 to 223608a Compare August 12, 2024 12:12
Copy link

ti-chi-bot bot commented Aug 12, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

This pull request seems to be related to the addition of a Kafka topic to the cloudevents-server application. The code changes are mostly related to the configuration and handling of Kafka events.

Key changes:

  1. Addition of Kafka related fields in the configuration struct (in config.go).
  2. Creation of Kafka event handlers (in kafka.go) and addition of the Kafka handling logic in the main server file (main.go).
  3. Addition of Kafka related dependencies in go.mod and go.sum.

Potential problems:

  1. There are many changes in the go.mod and go.sum files, including version upgrades for several dependencies. This can potentially break the application if there are incompatibilities with the new versions.
  2. It seems like error handling for the Kafka setup isn't entirely thorough. For instance, potential Kafka setup errors are just logged and not handled in other ways.
  3. The Kafka related configuration values are not validated. If they are incorrect or not provided, the application might crash or not work correctly.

Suggestions:

  1. The changes in the go.mod and go.sum files should be carefully reviewed and tested to make sure they don't break anything.
  2. Improve error handling for the Kafka setup. If setting up Kafka fails, the application should handle this appropriately, for instance by exiting with an error message.
  3. Add validation for the Kafka related configuration values to make sure they are correct and provided.
  4. Write unit tests to verify the new functionality and prevent regression in the future.
  5. Add comments to the new code to explain what it does, which would help future maintainers.

@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Sep 8, 2024

manual test Deno script:

import { describe, it, beforeAll } from "jsr:@std/testing/bdd";
import { httpTransport, emitterFor, CloudEvent, EmitterFunction } from "npm:cloudevents@8.0.2";


function loadCloudEvent(filename: string) {
  const content = Deno.readTextFileSync(filename);
  return new CloudEvent(JSON.parse(content))
}

describe("Send pipeline run events", () => {
  let emit: EmitterFunction
  beforeAll( () => {
     emit = emitterFor(httpTransport("http://localhost:8888/events"));
  })

  it("Send failed event", async () => {
    const eventFile = "pkg/events/custom/tekton/testdata/event-pipelinerun.failed.json";
    const event = loadCloudEvent(eventFile);
    await emit(event);
  });
});

@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Sep 8, 2024

/approve

Copy link

ti-chi-bot bot commented Sep 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

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

@ti-chi-bot ti-chi-bot bot added the approved label Sep 8, 2024
@ti-chi-bot ti-chi-bot bot merged commit bbd423a into main Sep 8, 2024
3 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feature/cloudevent-producer-and-consumer branch September 8, 2024 13:20
ti-chi-bot bot pushed a commit to PingCAP-QE/ee-ops that referenced this pull request Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant