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(apps/prod/tekton/setup): enable cloudevent emit and add sink url #825

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

wuhuizuo
Copy link
Collaborator

@wuhuizuo wuhuizuo commented Dec 6, 2023

Notice: currently we need update the config map tekton-pipelines/feature-flags manually because of the operator not support the congfiguration before v0.63.0.
Signed-off-by: wuhuizuo wuhuizuo@126.com

Notice: currently we need update the config map `tekton-pipelines/feature-flags` manually because of the operator not support the congfiguration before v0.63.0.
Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Copy link
Contributor

ti-chi-bot bot commented Dec 6, 2023

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

Pull Request Review

Summary

The pull request adds the ability to enable cloudevent emit and add sink url to the operator-config.yaml file.

Potential Problems

  • The pull request description mentions that the config map tekton-pipelines/feature-flags needs to be updated manually because the operator does not support the configuration before v0.63.0. It is not clear whether this has been addressed in the pull request or if it is still a problem.
  • The sink URL is hardcoded in the yaml file. It is recommended to use environment variables to allow for easier configuration changes in the future.

Fix Suggestions

  • If the problem with the config map is still present, it should be addressed in the pull request and documented in the description.
  • Use environment variables in place of the hardcoded URL.

@ti-chi-bot ti-chi-bot bot requested review from jayl1e and purelind December 6, 2023 10:21
@ti-chi-bot ti-chi-bot bot added area/apps env/prod will deploy on the main product cluster size/XS labels Dec 6, 2023
Copy link
Contributor

ti-chi-bot bot commented Dec 6, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
Based on the provided information, it seems that the pull request aims to add a new feature to the tekton-pipelines/feature-flags config map, namely to enable the emission of cloudevents and add a sink URL.

The key changes in the code are the addition of the send-cloudevents-for-runs: true flag, which enables the emission of cloudevents for pipeline runs, and the default-cloud-events-sink: http://cloudevents-server.apps.svc/events field, which sets the URL of the sink for the emitted cloudevents.

One potential problem with the pull request is that it mentions that the configuration map needs to be updated manually, which might be error-prone and could lead to unexpected behavior. A suggestion for fixing this issue would be to automate the update of the configuration map through a script or a webhook.

Another potential problem is that the sink URL is hard-coded in the configuration file, which might not be flexible enough in some cases. A suggestion for fixing this issue would be to make the sink URL configurable through environment variables or command-line arguments.

Overall, the pull request seems to add a useful feature to the tekton-pipelines/feature-flags config map, but some improvements could be made to make the configuration more robust and flexible.

@wuhuizuo
Copy link
Collaborator Author

wuhuizuo commented Dec 6, 2023

/approve

Copy link
Contributor

ti-chi-bot bot commented Dec 6, 2023

[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 Dec 6, 2023
@ti-chi-bot ti-chi-bot bot merged commit 5f1e4d7 into main Dec 6, 2023
4 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feature/tekton-send-cloud-events branch December 6, 2023 10:26
wuhuizuo added a commit that referenced this pull request Dec 6, 2023
…#825)

Notice: currently we need update the config map
`tekton-pipelines/feature-flags` manually because of the operator not
support the congfiguration before v0.63.0.
Signed-off-by: wuhuizuo <wuhuizuo@126.com>

---------

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
wuhuizuo added a commit that referenced this pull request Dec 6, 2023
…#825)

Notice: currently we need update the config map
`tekton-pipelines/feature-flags` manually because of the operator not
support the congfiguration before v0.63.0.
Signed-off-by: wuhuizuo <wuhuizuo@126.com>

---------

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
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 size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant