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: add monitoring trigger #924

Merged
merged 4 commits into from
Jan 15, 2024
Merged

feat: add monitoring trigger #924

merged 4 commits into from
Jan 15, 2024

Conversation

jayl1e
Copy link
Contributor

@jayl1e jayl1e commented Jan 4, 2024

No description provided.

jayl1e added 2 commits January 4, 2024 17:50
Signed-off-by: lijie <lijie@pingcap.com>
Signed-off-by: lijie <lijie@pingcap.com>
@ti-chi-bot ti-chi-bot bot requested review from purelind and wuhuizuo January 4, 2024 09:52
@ti-chi-bot ti-chi-bot bot added area/apps env/prod will deploy on the main product cluster labels Jan 4, 2024
Copy link
Contributor

ti-chi-bot bot commented Jan 4, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.
Pull Request Review: feat: add monitoring trigger

Summary

The pull request adds monitoring triggers to the tekton pipeline. The changes include the addition of four new YAML files, which define the behavior of the triggers. Two of these files are used for creating tags, and the other two are used for pushing changes and performing builds.

Potential Problems

  • The timeout parameter is set to 30 minutes for all bindings, which might be too long for some builds. It is better to set this parameter according to the expected build time.
  • The builder-resources-memory and builder-resources-cpu parameters are set to high values, which might cause the pipeline to exceed available resources. It is better to test these values carefully before deploying the pipeline.
  • There are no tests included in the pull request, which makes it difficult to verify that the changes work as intended.

Fixing Suggestions

  • Set the timeout parameter to a more reasonable value.
  • Test the builder-resources-memory and builder-resources-cpu parameters and adjust them according to the available resources.
  • Add tests to verify that the pipeline works as intended.

Overall

The changes in the pull request look good, but there are some potential problems that need to be addressed. It is recommended to fix these issues before merging the pull request.

Copy link
Contributor

ti-chi-bot bot commented Jan 4, 2024

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

PR Summary

This PR adds monitoring triggers to the codebase. It includes four new YAML files, each containing the configuration for a different trigger. The triggers are related to creating new tags and pushing to branches on GitHub. The triggers use Tekton's interceptors and bindings features to filter on specific repositories and branches, and to specify values for various parameters.

Potential Problems

  • The description of the PR is empty, which makes it difficult to understand the context and motivation behind the changes.
  • There is no explanation of how the triggers work, how to use them, or what their purpose is.
  • The names of some of the parameters (timeout, source-ws-size, builder-resources-memory, etc.) are not self-explanatory and could be confusing to someone unfamiliar with the Tekton framework.
  • There are no tests included with the code changes, so it is difficult to verify that the triggers work correctly.

Fixing Suggestions

  • The PR author should provide a brief description of the problem that the triggers are solving, and how they fit into the overall architecture of the codebase. This information will help reviewers understand the purpose of the changes.
  • The PR author should provide documentation for the triggers, including how to use them and what their purpose is. This documentation could be included in the PR description or as a separate document in the repository.
  • The PR author should consider renaming the parameters to be more self-explanatory, or provide additional documentation explaining what they mean.
  • The PR author should consider adding tests for the triggers to ensure that they work as intended.

@ti-chi-bot ti-chi-bot bot added the size/L label Jan 4, 2024
Signed-off-by: lijie <lijie@pingcap.com>
Copy link
Contributor

ti-chi-bot bot commented Jan 4, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.
Based on the pull request diff, it seems that the changes add some new YAML config files for Tekton triggers that relate to monitoring and Git operations. Specifically, the changes add four new files in the apps/prod/tekton/configs/triggers/triggers/pingcap/monitoring directory, which are:

  • fake-git-create-tag.yaml
  • fake-git-push.yaml
  • git-create-tag.yaml
  • git-push.yaml

These files define how Tekton triggers should react when certain events occur in the Git repository for the PingCAP monitoring application. The changes also specify some parameters that determine the resources allocated to the Tekton pipeline for building and deploying the application.

It is difficult to identify potential problems without context about the purpose and design of the changes. However, there are some general suggestions that could improve the quality of the pull request:

  • Add a detailed description to the pull request to explain the motivation and goals of the changes. This can help reviewers understand the context and provide more specific feedback.
  • Consider adding comments to the YAML files to explain the purpose of each section and how it contributes to the overall workflow. This can help other contributors understand the code and make modifications in the future.
  • Check that the changes conform to any coding standards or conventions established by the project. This could include formatting, naming, or other guidelines.
  • Review the resource allocation parameters to ensure that they are appropriate for the application and the target environment. This can help prevent performance or stability issues.

Overall, the changes seem to be adding functionality to the PingCAP monitoring application, but more context would be helpful to fully evaluate the quality of the pull request.

@wuhuizuo
Copy link
Collaborator

wuhuizuo commented Jan 4, 2024

/hold

Signed-off-by: lijie <lijie@pingcap.com>
@ti-chi-bot ti-chi-bot bot removed the lgtm label Jan 4, 2024
@ti-chi-bot ti-chi-bot bot added the lgtm label Jan 9, 2024
Copy link
Contributor

ti-chi-bot bot commented Jan 9, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-01-04 10:38:37.14711113 +0000 UTC m=+2339808.184338058: ☑️ agreed by wuhuizuo.
  • 2024-01-04 10:41:27.267485843 +0000 UTC m=+2339978.304712771: ✖️🔁 reset by lijie0123.
  • 2024-01-09 10:13:23.411407881 +0000 UTC m=+351792.995661619: ☑️ agreed by wuhuizuo.

@wuhuizuo
Copy link
Collaborator

/approve

Copy link
Contributor

ti-chi-bot bot commented Jan 15, 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

@wuhuizuo
Copy link
Collaborator

/unhold

@ti-chi-bot ti-chi-bot bot merged commit 81f13bd into main Jan 15, 2024
4 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feat/monitoring branch January 15, 2024 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/apps env/prod will deploy on the main product cluster lgtm size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants