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(publisher): pretty the notify lark message #200

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

wuhuizuo
Copy link
Contributor

@wuhuizuo wuhuizuo commented Nov 7, 2024

Signed-off-by: wuhuizuo [email protected]

@ti-chi-bot ti-chi-bot bot requested a review from purelind November 7, 2024 12:32
Copy link

ti-chi-bot bot commented Nov 7, 2024

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

This pull request adds the functionality to send a pretty message to Lark when publishing a package fails. It also includes a new test case for it.

Potential problems:

  • The webhook URL is hardcoded in the code, which is not ideal for maintainability and scalability. It is recommended to extract it to a configuration file or environment variables.
  • The function newLarkCardWithGoTemplate parses a YAML file and returns a JSON string. It may cause unexpected errors if the YAML file format is incorrect. It is recommended to add some error handling code to avoid potential issues.

Fixing suggestions:

  • Extract the webhook URL to a configuration file or environment variables.
  • Add some error handling code in the function newLarkCardWithGoTemplate to catch any errors that might occur.

@ti-chi-bot ti-chi-bot bot added the size/XL label Nov 7, 2024
@wuhuizuo wuhuizuo force-pushed the feature/add-rerun-instructor-for-failure branch from c80106c to 5b3fb38 Compare November 8, 2024 12:32
Copy link

ti-chi-bot bot commented Nov 8, 2024

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

The pull request makes the following changes:

  1. Adds new dependencies to the go.mod and go.sum files. The added dependencies include github.com/Masterminds/sprig/v3, dario.cat/mergo, github.com/Masterminds/goutils, github.com/Masterminds/semver/v3, github.com/huandu/xstrings, github.com/mitchellh/copystructure, github.com/mitchellh/reflectwalk, github.com/shopspring/decimal, github.com/spf13/cast, and golang.org/x/crypto.

  2. Adds a new file lark.go under the impl package. The file contains a couple of new functions including newLarkCardWithGoTemplate which creates a new Lark card using Go templates and failureNotifyInfo which is a struct type for failure notification information.

  3. Adds a new file tiup-publish-failure-notify.yaml.tmpl under the lark_templates directory. This file appears to be a template for a failure notification message.

  4. Modifies the tiup_worker.go file. The changes include modifying the notifyLark function to accept PublishRequest and error as arguments instead of PublishInfo and error. The notifyLark function is also updated to send a more detailed failure notification message using the new template.

  5. Adds a new test file tiup_worker_test.go for the tiupWorker struct. However, the tests are currently skipped.

  6. Updates the types.go file to include a String method for From and FromOci types.

Potential problems and suggestions:

  1. Ensure that all the newly added dependencies are necessary for the project. Unnecessary dependencies can increase the project's size and potential attack surface.

  2. The new test file tiup_worker_test.go contains skipped tests. These tests should be enabled and passing before the pull request is merged.

  3. Make sure the new template tiup-publish-failure-notify.yaml.tmpl is correct and matches the required format.

  4. The notifyLark function now creates a more detailed failure notification message. Ensure that this detailed message does not expose any sensitive information.

  5. In tiup_worker.go, the default value of PublicServiceURL is set to http://publisher-<env>-mirror.<namespace>.svc. If this default value is not suitable for all environments, consider changing the approach to setting this value.

  6. In the types.go file, the String method is added for the From and FromOci types. Make sure that the String method is returning the expected string format.

@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Nov 8, 2024

/approve

Copy link

ti-chi-bot bot commented Nov 9, 2024

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@wuhuizuo wuhuizuo force-pushed the feature/add-rerun-instructor-for-failure branch from 5b3fb38 to 784efe3 Compare November 12, 2024 04:35
Copy link

ti-chi-bot bot commented Nov 12, 2024

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

Pull Request Title: feat(publisher): pretty the notify lark message

The key changes introduced in this Pull Request include:

  1. Addition of new dependencies in go.mod and go.sum files such as github.com/Masterminds/sprig/v3 v3.3.0 and several indirect dependencies.
  2. Addition of a new file named lark.go in pkg/impl directory. This file contains a new function newLarkCardWithGoTemplate() that potentially handles the pretty formatting of the lark message.
  3. Addition of a new yaml template for lark messages tiup-publish-failure-notify.yaml.tmpl.
  4. Modification of tiup_worker.go file to include the use of newLarkCardWithGoTemplate() function for pretty formatting of lark messages.
  5. Addition of a new test file tiup_worker_test.go which includes tests for the new changes.

Potential Problems:

  1. The new dependencies added might have compatibility issues with the existing codebase or might introduce unexpected behavior.
  2. The new function newLarkCardWithGoTemplate() is not tested. If there is an issue with this function, it could potentially break the formatting of lark messages.
  3. The new yaml template file for lark messages should be correctly formatted, else it could lead to errors.
  4. Changes in tiup_worker.go are not covered with tests. This might lead to potential bugs which are hard to track.
  5. The new test file tiup_worker_test.go might have insufficient coverage or might not properly test the new changes.

Suggestions for Fixes:

  1. Ensure the compatibility and stability of the newly added dependencies. Run integration and unit tests to check their impact on the existing codebase.
  2. Write comprehensive tests for the new function newLarkCardWithGoTemplate() to ensure its correct functionality.
  3. Validate the new yaml template file for correct syntax and formatting.
  4. Add tests to cover the changes made in tiup_worker.go file.
  5. Review and improve the test cases in the new test file tiup_worker_test.go for better coverage and effectiveness.

@wuhuizuo
Copy link
Contributor Author

please update the pr label

@wuhuizuo
Copy link
Contributor Author

/approve

1 similar comment
@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Nov 12, 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 merged commit 0c2a438 into main Nov 12, 2024
3 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feature/add-rerun-instructor-for-failure branch November 12, 2024 08:38
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