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

feat: implement tiup publisher #172

Merged
merged 5 commits into from
Sep 30, 2024

Conversation

wuhuizuo
Copy link
Contributor

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

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Copy link

ti-chi-bot bot commented Sep 30, 2024

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

The Pull Request implements a new feature for a tiup publisher, as indicated by the "feat:" prefix in the title. In the PR, numerous changes have been made to multiple files.

Key changes include:

  1. The Go version is upgraded from 1.22.4 to 1.23.1, and several dependencies are updated in cloudevents-server/go.mod and cloudevents-server/go.sum.
  2. In cloudevents-server/Dockerfile, the Go version for the builder is updated from 1.22.4 to 1.23.1.
  3. Several files are moved, for example, from cloudevents-server/pkg/events/custom/tekton to cloudevents-server/pkg/events/tekton.
  4. A new directory tiup-publisher is introduced with several new Go files, including handler.go, main.go, and types.go.

Potential problems:

  1. The Go version upgrade might cause compatibility issues. If the project relies on features or behaviors in the older version, it could cause issues.
  2. Some of the dependencies have been removed from the require section of cloudevents-server/go.mod. If these dependencies are still required, this will cause compilation errors.
  3. There are large changes in cloudevents-server/handlers.go, cloudevents-server/pkg/config/config.go, and other files. If these changes are not properly reviewed and tested, it might introduce bugs.

Fixing suggestions:

  1. The Go version upgrade should be tested thoroughly to ensure it doesn't break the existing application. It's also recommended to read the Go release note to understand the changes in the new version.
  2. The removed dependencies in go.mod should be reviewed, and the project should be built and tested to make sure no required dependencies are missing.
  3. The changes in the PR should be reviewed carefully and tested thoroughly. It's recommended to write unit tests and integration tests to ensure the new changes work as expected.

Copy link

ti-chi-bot bot commented Sep 30, 2024

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

This pull request has quite a few changes. Here is a summary of the key changes:

  1. It introduces a new feature, the tiup publisher, which is implemented in Go. The new files for this feature include tiup-publisher/main.go, tiup-publisher/handler.go, tiup-publisher/handler_test.go, tiup-publisher/types.go, and tiup-publisher/skaffold.yaml, amongst others.

  2. The pull request also modifies a few existing files, mainly Dockerfiles and Go module files (go.mod and go.sum). The Dockerfiles have been updated to use a newer version of Go (from golang:1.22.4 to golang:1.23.1). The Go module files have been updated with new dependencies and versions.

  3. The pull request introduces new Helm charts for the tiup-publisher. These include .helmignore, Chart.yaml, NOTES.txt, _helpers.tpl, deployment.yaml, hpa.yaml, ingress.yaml, serviceaccount.yaml, test-connection.yaml, and values.yaml.

  4. The pull request moves some Go files from the custom directory to the root of their respective directories. This includes files in the tekton and tibuild directories.

Potential problems:

  1. The pull request does not seem to include any unit tests for the new feature. This can be a problem if the feature has not been adequately tested.
  2. The pull request includes changes to multiple different parts of the codebase (new feature, Dockerfile updates, Go module updates, Helm chart additions, file moves). This can make it harder to review and test the pull request.
  3. The pull request modifies the Go module files (go.mod and go.sum) but it's unclear if the go.sum file has been updated correctly. This file should be automatically updated by running go mod tidy.

Suggestions:

  1. Consider adding unit tests for the new feature to ensure it works as expected.
  2. Consider splitting the pull request into smaller pull requests, each with a single purpose (new feature, Dockerfile updates, Go module updates, Helm chart additions, file moves). This can make it easier to review and test the changes.
  3. Ensure the go.sum file is up-to-date by running go mod tidy.

@wuhuizuo wuhuizuo force-pushed the feature/cloudevents-server-publish-tiup branch from 6aaf652 to e800976 Compare September 30, 2024 09:10
Copy link

ti-chi-bot bot commented Sep 30, 2024

I Skip it since the diff size(84021 bytes > 80000 bytes) is too large

@wuhuizuo wuhuizuo force-pushed the feature/cloudevents-server-publish-tiup branch from e800976 to 8171d06 Compare September 30, 2024 09:10
Copy link

ti-chi-bot bot commented Sep 30, 2024

I Skip it since the diff size(85994 bytes > 80000 bytes) is too large

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@wuhuizuo wuhuizuo force-pushed the feature/cloudevents-server-publish-tiup branch from 8171d06 to e42c88d Compare September 30, 2024 09:19
Copy link

ti-chi-bot bot commented Sep 30, 2024

I Skip it since the diff size(85975 bytes > 80000 bytes) is too large

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Copy link

ti-chi-bot bot commented Sep 30, 2024

I Skip it since the diff size(118590 bytes > 80000 bytes) is too large

Copy link

ti-chi-bot bot commented Sep 30, 2024

I Skip it since the diff size(118611 bytes > 80000 bytes) is too large

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@wuhuizuo wuhuizuo force-pushed the feature/cloudevents-server-publish-tiup branch from f7b1893 to dfb0818 Compare September 30, 2024 09:53
Copy link

ti-chi-bot bot commented Sep 30, 2024

I Skip it since the diff size(117091 bytes > 80000 bytes) is too large

@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Sep 30, 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 30, 2024
@ti-chi-bot ti-chi-bot bot merged commit 79220a3 into main Sep 30, 2024
3 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feature/cloudevents-server-publish-tiup branch September 30, 2024 10:56
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