[TEP-0137] Add events config map #6883

afrittoli · 2023-06-27T22:08:33Z

Changes

Add a new "events" config map. The config map supersedes the cloudevent settings from the "defaults" config map. The current settings is deprecated but still honoured, so the change is transparent for existing users. They shall migrate to the new settings before the old one is removed.

This change is in preparation for TEP-0137, where new format of events will be introduced, and configured via the new events configuration map.

The current config map is only read from within the events package, so the change to the logic is localised there, it only propagates into tests.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

action required: The `default-cloud-events-sink` setting in the `config-defaults` config map is deprecated. The CloudEvents sink shall be configured now through the `sink` settings in the new `config-events` config map.

/kind feature

tekton-robot · 2023-06-27T22:15:28Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/events.go	Do not exist	98.1%
pkg/apis/config/store.go	91.7%	92.6%	0.9
test/controller.go	29.4%	30.1%	0.7

tekton-robot · 2023-06-27T22:15:58Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/events.go	Do not exist	98.1%
pkg/apis/config/store.go	91.7%	92.6%	0.9
test/controller.go	29.4%	30.1%	0.7

tekton-robot · 2023-06-27T22:19:45Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/events.go	Do not exist	98.1%
pkg/apis/config/store.go	91.7%	92.6%	0.9
test/controller.go	29.4%	30.1%	0.7

tekton-robot · 2023-06-28T09:35:14Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/events.go	Do not exist	98.1%
pkg/apis/config/store.go	91.7%	92.6%	0.9
pkg/reconciler/events/cloudevent/cloud_event_controller.go	84.9%	84.6%	-0.3
test/controller.go	29.4%	30.1%	0.7

tekton-robot · 2023-06-28T09:39:01Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/events.go	Do not exist	98.1%
pkg/apis/config/store.go	91.7%	92.6%	0.9
pkg/reconciler/events/cloudevent/cloud_event_controller.go	84.9%	84.6%	-0.3
test/controller.go	29.4%	30.1%	0.7

config/config-events.yaml

lbernick · 2023-06-28T12:53:55Z

pkg/apis/config/events.go

+)
+
+var (
+	// Note(afrittoli): only one valid format for now, more to come


could you link an issue as the TODO here?

lbernick · 2023-06-28T12:59:30Z

pkg/apis/config/events.go

+type EventFormat string
+
+// EventFormats is a set of event formats
+type EventFormats map[EventFormat]struct{}


nit: could https://pkg.go.dev/k8s.io/[email protected]/pkg/util/sets#Set be used here? It looks like the main thing missing is just a string representation.

Yes, I'm pretty sure we could use that.
Since I use very little of the Set functionality I thought it was not needed, but it would at least save me the need for the Equals method.

I tried a bit, but go stuck trying to wrap my own type around the Set.
I've check Set implementation and it's a map[T]Empty where Empty is struct{} - so identically to what I have except for the "generics" bit, which is not needed here, so I think I'll stick to the current implementation this time.

lbernick · 2023-06-28T13:02:26Z

docs/additional-configs.md

+```
+
+The sink used to be configured in the `config-defaults` config map.
+This option is still available, but deprecated, and will be removed.


could you add this to the deprecations table?

I definitely will. I was a bit put off by the fact that I need to put things like date and release number in there, but I can start with a basic entry and update it once things are merged and released

Done - deprecating a config parameter in favour of a new one that provides the same functionality is not really covered (or in the scope of) the API compatibility policy. Since the config option has been around for a while and there's no real extra maintenance burden in keeping it around, I marked it for removal in 9 months

Yongxuanzhang

we can merge once Lee's comments are addressed

pkg/apis/config/events_test.go

Add a new "events" config map. The config map supersedes the cloudevent settings from the "defaults" config map. The current settings is deprecated but still honoured, so the change is transparent for existing users. They shall migrate to the new settings before the old one is removed. This change is in preparation for TEP-0137, where new format of events will be introduced, and configured via the new events configuration map. The current config map is only read from within the events package, so the change to the logic is localised there, it only propagates into tests. Signed-off-by: Andrea Frittoli <[email protected]>

tekton-robot · 2023-06-30T09:16:51Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/events.go	Do not exist	98.1%
pkg/apis/config/store.go	91.7%	92.6%	0.9
pkg/reconciler/events/cloudevent/cloud_event_controller.go	84.9%	84.6%	-0.3
test/controller.go	29.4%	30.1%	0.7

tekton-robot · 2023-06-30T09:22:05Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/events.go	Do not exist	98.1%
pkg/apis/config/store.go	91.7%	92.6%	0.9
pkg/reconciler/events/cloudevent/cloud_event_controller.go	84.9%	84.6%	-0.3
test/controller.go	29.4%	30.1%	0.7

vdemeester

/lgtm

tekton-robot · 2023-07-03T07:38:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick, vdemeester, Yongxuanzhang

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:

~~OWNERS~~ [lbernick,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. kind/feature Categorizes issue or PR as related to a new feature. labels Jun 27, 2023

tekton-robot requested review from abayer and pritidesai June 27, 2023 22:08

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 27, 2023

afrittoli force-pushed the tep0137_events branch from f586d36 to b3ec85b Compare June 27, 2023 22:09

afrittoli force-pushed the tep0137_events branch from b3ec85b to 1caa5e0 Compare June 28, 2023 09:29

afrittoli mentioned this pull request Jun 28, 2023

[TEP-0137] Rename customrun package #6884

Merged

8 tasks

lbernick approved these changes Jun 28, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2023

afrittoli mentioned this pull request Jun 29, 2023

[TEP-0137] Restructure customrun event controller #6889

Open

9 tasks

Yongxuanzhang approved these changes Jun 29, 2023

View reviewed changes

pkg/apis/config/events_test.go Outdated Show resolved Hide resolved

afrittoli force-pushed the tep0137_events branch from 1caa5e0 to 8950ade Compare June 30, 2023 09:11

afrittoli added kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). and removed kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). labels Jun 30, 2023

vdemeester approved these changes Jul 3, 2023

View reviewed changes

tekton-robot assigned vdemeester Jul 3, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2023

tekton-robot merged commit b22070b into tektoncd:main Jul 3, 2023

afrittoli mentioned this pull request Jul 10, 2023

[TEP-0137] Rework the events controller cache #6914

Open

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEP-0137] Add events config map #6883

[TEP-0137] Add events config map #6883

afrittoli commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 28, 2023

tekton-robot commented Jun 28, 2023

lbernick Jun 28, 2023

afrittoli Jun 30, 2023

lbernick Jun 28, 2023

afrittoli Jun 28, 2023

afrittoli Jun 30, 2023

lbernick Jun 28, 2023

afrittoli Jun 28, 2023 •

edited

Loading

afrittoli Jun 30, 2023

Yongxuanzhang left a comment

tekton-robot commented Jun 30, 2023

tekton-robot commented Jun 30, 2023

vdemeester left a comment

tekton-robot commented Jul 3, 2023

[TEP-0137] Add events config map #6883

[TEP-0137] Add events config map #6883

Conversation

afrittoli commented Jun 27, 2023

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 28, 2023

tekton-robot commented Jun 28, 2023

lbernick Jun 28, 2023

Choose a reason for hiding this comment

afrittoli Jun 30, 2023

Choose a reason for hiding this comment

lbernick Jun 28, 2023

Choose a reason for hiding this comment

afrittoli Jun 28, 2023

Choose a reason for hiding this comment

afrittoli Jun 30, 2023

Choose a reason for hiding this comment

lbernick Jun 28, 2023

Choose a reason for hiding this comment

afrittoli Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

afrittoli Jun 30, 2023

Choose a reason for hiding this comment

Yongxuanzhang left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 30, 2023

tekton-robot commented Jun 30, 2023

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 3, 2023

afrittoli Jun 28, 2023 •

edited

Loading