SRVCLI-401: 🎁 Add kn-event configuration #154

cardil · 2024-05-09T15:52:48Z

Changes

🎁 Add kn-event configuration

Here's the current config diff for kn-event: https://gist.github.com/cardil/adedaf1313446a6329fb9a958670198a (I'll keep it updated)

openshift-ci · 2024-05-09T15:52:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cardil
Once this PR has been reviewed and has the lgtm label, please assign skonto for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

openshift-ci-robot · 2024-05-09T15:55:21Z

@cardil: This pull request references SRVCLI-401 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Changes

🎁 Add kn-event configuration

Fixes SRVCLI-401

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 openshift-eng/jira-lifecycle-plugin repository.

cardil · 2024-05-09T15:56:56Z

/kind enhancement

cardil · 2024-05-10T18:53:08Z

Require openshift-knative/kn-plugin-event#401

dsimansk · 2024-05-15T09:37:48Z

config/kn-event.yaml

+repositories:
+  - org: openshift-knative
+    repo: kn-plugin-event
+    imagePrefix: client-plugin-event


It's a bit nit picking, but I wonder how the final names is generated from all those values. Is it going too verbose? I'd keep kn-plugin-event here. Wdyt?

This is how they used to be called: https://github.com/openshift/release/blob/a45d8237303d117149c08a5c4c56a558fd62dfbf/ci-operator/config/openshift-knative/kn-plugin-event/openshift-knative-kn-plugin-event-release-next__415.yaml#L74-L79

However, I don't know if I'll be able to keep the current naming, and will be force to adjust kn-event to follow hack's machinery.

This is what I am currently getting (WIP):

test: - as: test + cli: latest commands: openshift/e2e-tests.sh dependencies: - - env: TEST_IMAGES_EVENTSHUB + - env: CLIENT_PLUGIN_EVENT_BUILD_IMAGE + name: client-plugin-event-build-image + - env: CLIENT_PLUGIN_EVENT_SENDER + name: client-plugin-event-sender + - env: CLIENT_PLUGIN_EVENT_SOURCE_IMAGE + name: client-plugin-event-source-image + - env: CLIENT_PLUGIN_EVENT_TEST_EVENTSHUB name: client-plugin-event-test-eventshub - - env: TEST_IMAGES_WATHOLA_FORWARDER + - env: CLIENT_PLUGIN_EVENT_TEST_WATHOLA_FORWARDER name: client-plugin-event-test-wathola-forwarder - - env: IMAGES_KN_EVENT_SENDER - name: client-plugin-event-sender - from: src + from: client-plugin-event-source-image resources: requests: cpu: 100m timeout: 30m0s

Here's a full diff, if you're interested: https://gist.github.com/cardil/adedaf1313446a6329fb9a958670198a

I updated the diff above. I think it's acceptable to proceed.

The naming thing. Yes, we could proceed with an image prefix of kn-plugin-event, however:

historically, the images were published under the prefix client-plugin-event, so keeping it makes the history consistent

we already burned down the naming convention in [release-1.14] 🧹 o-k/hack naming convention for images kn-plugin-event#442 (and its backports)

So, we can change the image prefix to kn-plugin-event, but I'm a bit reluctant due to the above points.

dsimansk · 2024-05-15T09:41:10Z

config/kn-event.yaml

+    binaryBuildCommands: |-
+      export HOME=/tmp
+      export TEST_IMAGES_DIR=/usr/bin
+      ./mage build


In case of client, I've been looking at this stanza and my thought's been to wrap it into Dockerfile stage build to simplify CI config. Similarly to Makefile PR in the kn-plugin-event midstream, but also execute the make withing Docker builds.

This is how Openshift CI advises building binaries in a separate step, and later use the bin image contents, while building the images.

Since then, most Serverless repos moved to pure Docker approach, when the bin building doesn't matter, as everything is done in the build image step: https://github.com/openshift-knative/serving/blob/release-next/openshift/ci-operator/knative-images/activator/Dockerfile

IDK, if I keep kn-event current setup or shift it to how other repos does this. I'm leaning towards the pure Dockerfile approach.

I decided to keep the current approach, as I managed to change the code to support both ways. Also, the current kn-event's approach may be better for multi-arch builds in Openshift CI. The current CI setup for kn-event works, and changing it just for the sake of bringing it on par with other repos seems pointless.

Still, maybe, I'll follow up the kn-event to add a Makefile target that detects Openshift CI environment and sets proper environment variables.

dsimansk · 2024-05-15T09:43:32Z

config/serverless-operator.yaml

+    env:
+      SKIP_MESH_AUTH_POLICY_GENERATION: "true"


Is it related to work on event plugin? Would it make more sense to check in S-O Makefile if PROW and based on that define defaults skips? Maybe we will need more skips in the future.

This is a rather messy env var introduced in #77

Now, it is added to every command. I didn't like that, so I added support for defining the environmental variables.

I believe this variable makes sense only in terms of S-O, that's why I added it just to its config. Is it right, @skonto?

dsimansk · 2024-05-15T09:46:06Z

Makefile

-generate-ci-no-clean: generate-eventing-ci generate-serving-ci generate-serverless-operator-ci
-.PHONY: generate-ci-no-clean
-
-generate-eventing-ci:


These cmd name changes will most likely break GitHub actions that execute generators periodically.

I'll update the workflows in this repo. Are you aware of other cross-repo references to those targets?

The actions are updated, and I restored those targets, as deprecated (they print a warn box when used).

I'd defer name change to @mgencur or @pierDipi to chime in here.
I'm not a huge fan to carry this change within "add config" PR, because it doesn't seem to me as necessary for adding event plugin config.

openshift-ci-robot · 2024-05-15T13:39:35Z

@cardil: This pull request references SRVCLI-401 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Changes

🎁 Add kn-event configuration

Fixes SRVCLI-401

Here's the current config diff for kn-event: https://gist.github.com/cardil/adedaf1313446a6329fb9a958670198a (I'll keep it updated)

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 openshift-eng/jira-lifecycle-plugin repository.

**Conflicts fixed:** - pkg/prowgen/prowgen.go - pkg/prowgen/prowgen_images_discovery.go

Conflicts fixed: - Makefile

cardil · 2024-06-19T11:03:22Z

/hold

Let me provide the full diff, for all the components...

EDIT: The gist now contains the full diff, of all the components: https://gist.github.com/cardil/adedaf1313446a6329fb9a958670198a

The diff needs to be reviewed...

dsimansk · 2024-06-19T11:10:23Z

pkg/prowgen/prowgen_config.go

 	ImagePrefix           string                                                      `json:"imagePrefix,omitempty" yaml:"imagePrefix,omitempty"`
 	ImageNameOverrides    map[string]string                                           `json:"imageNameOverrides,omitempty" yaml:"imageNameOverrides,omitempty"`
 	SlackChannel          string                                                      `json:"slackChannel,omitempty" yaml:"slackChannel,omitempty"`
 	CanonicalGoRepository *string                                                     `json:"canonicalGoRepository,omitempty" yaml:"canonicalGoRepository,omitempty"`
 	E2ETests              []E2ETest                                                   `json:"e2e,omitempty" yaml:"e2e,omitempty"`
 	Dockerfiles           Dockerfiles                                                 `json:"dockerfiles,omitempty" yaml:"dockerfiles,omitempty"`
+	BaseImages            map[string]cioperatorapi.ImageStreamTagReference            `json:"baseImages,omitempty" yaml:"baseImages,omitempty"`
+	SharedInputs          map[string]cioperatorapi.ImageBuildInputs                   `json:"sharedInputs,omitempty" yaml:"sharedInputs,omitempty"`


Wouldn't it make sense to keep variable name as per CI operator API, ImageBuildInputs?

Yes, I can name it ImageBuildInputs although the shared aspect differs here from CI operator API.

In CI operator API, the ImageBuildInputs is used for a specific image. Here, I'm using that do add common, provided upfront inputs for all images from a given repo.

I could name it SharedImageBuildInputs, but isn't that too mouthful?

pkg/prowgen/prowgen_config.go

dsimansk · 2024-06-19T11:17:46Z

pkg/prowgen/prowgen_images_discovery.go

+	Org    string
+	Repo   string
+	Tag    string
+	Digest string


Why is digest needed here?

We don't use it currently, which doesn't mean at any point such image reference might be used in one of Dockerfiles.

Conflicts fixed: - Makefile - pkg/prowgen/prowgen.go

Co-authored-by: David Simansky <dsimansk@redhat.com>

openshift-merge-robot · 2024-06-20T11:28:33Z

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.

openshift-ci bot requested review from aliok and pierDipi May 9, 2024 15:52

cardil marked this pull request as draft May 9, 2024 15:53

openshift-ci bot added the do-not-merge/work-in-progress label May 9, 2024

cardil changed the title ~~🎁 Add kn-event configuration~~ [WIP] 🎁 Add kn-event configuration May 9, 2024

cardil changed the title ~~[WIP] 🎁 Add kn-event configuration~~ [WIP] 🎁 SRVCLI-401 Add kn-event configuration May 9, 2024

cardil changed the title ~~[WIP] 🎁 SRVCLI-401 Add kn-event configuration~~ [WIP] SRVCLI-401: 🎁 Add kn-event configuration May 9, 2024

openshift-ci bot added the kind/enhancement New feature or request label May 9, 2024

Add kn-event configuration

5921c33

cardil force-pushed the feature/kn-event branch from 48c7253 to 5921c33 Compare May 10, 2024 18:52

Support for manually defined e2e tests, plus environment variables

2db6d7b

dsimansk reviewed May 15, 2024

View reviewed changes

Shared inputs for images

69c0a53

cardil mentioned this pull request Jun 14, 2024

🧹 Clean up kn-event CI config to align to o-k/h automation openshift/release#53227

Merged

cardil added 2 commits June 14, 2024 14:06

Add default excludes for discovered images

c6d0b8f

Commands and steps are exclusive for tests block

4dd2317

cardil mentioned this pull request Jun 14, 2024

[release-1.14] 🧹 o-k/hack naming convention for images openshift-knative/kn-plugin-event#442

Merged

Support base images provided upfront

e1d2874

openshift-merge-robot added the needs-rebase The PR needs to have conflicts fixed label Jun 18, 2024

Merge remote-tracking branch 'upstream/main' into feature/kn-event

eca04ba

**Conflicts fixed:** - pkg/prowgen/prowgen.go - pkg/prowgen/prowgen_images_discovery.go

openshift-merge-robot added needs-rebase The PR needs to have conflicts fixed and removed needs-rebase The PR needs to have conflicts fixed labels Jun 18, 2024

cardil force-pushed the feature/kn-event branch 4 times, most recently from 36fd7a7 to f076418 Compare June 18, 2024 18:54

Fixing unit tests

a503baf

cardil force-pushed the feature/kn-event branch from f076418 to a503baf Compare June 18, 2024 18:56

Merge remote-tracking branch 'upstream/main' into feature/kn-event

3db7212

Conflicts fixed: - Makefile

openshift-merge-robot removed the needs-rebase The PR needs to have conflicts fixed label Jun 18, 2024

cardil added 2 commits June 18, 2024 21:17

Format to milimize PR changes

f7bd3ce

Change the make targets in github workflows

e4c0286

cardil marked this pull request as ready for review June 18, 2024 19:27

openshift-ci bot requested review from rhuss and skonto June 18, 2024 19:27

cardil changed the title ~~[WIP] SRVCLI-401: 🎁 Add kn-event configuration~~ SRVCLI-401: 🎁 Add kn-event configuration Jun 18, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Jun 18, 2024

cardil requested a review from dsimansk June 18, 2024 19:30

openshift-ci bot added the do-not-merge/hold label Jun 19, 2024

dsimansk reviewed Jun 19, 2024

View reviewed changes

pkg/prowgen/prowgen_config.go Outdated Show resolved Hide resolved

dsimansk reviewed Jun 19, 2024

View reviewed changes

cardil and others added 4 commits June 19, 2024 13:52

Sort tests by name

766831b

Merge remote-tracking branch 'upstream/main' into feature/kn-event

5c6525b

Conflicts fixed: - Makefile - pkg/prowgen/prowgen.go

Skip scratch image alias

9f27b9e

Update pkg/prowgen/prowgen_config.go

f837021

Co-authored-by: David Simansky <dsimansk@redhat.com>

cardil force-pushed the feature/kn-event branch from e2dcaa5 to f837021 Compare June 19, 2024 12:36

openshift-merge-robot added the needs-rebase The PR needs to have conflicts fixed label Jun 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SRVCLI-401: 🎁 Add kn-event configuration #154

SRVCLI-401: 🎁 Add kn-event configuration #154

cardil commented May 9, 2024 •

edited

Loading

openshift-ci bot commented May 9, 2024

openshift-ci-robot commented May 9, 2024

Changes

cardil commented May 9, 2024

cardil commented May 10, 2024

dsimansk May 15, 2024

cardil May 15, 2024

cardil May 15, 2024

cardil Jun 18, 2024

dsimansk May 15, 2024

cardil May 15, 2024

cardil Jun 18, 2024 •

edited

Loading

dsimansk May 15, 2024

cardil May 15, 2024

dsimansk May 15, 2024

cardil May 15, 2024

cardil Jun 18, 2024

dsimansk Jun 19, 2024

openshift-ci-robot commented May 15, 2024

Changes

cardil commented Jun 19, 2024 •

edited

Loading

dsimansk Jun 19, 2024

cardil Jun 19, 2024

dsimansk Jun 19, 2024

cardil Jun 19, 2024

openshift-merge-robot commented Jun 20, 2024

SRVCLI-401: 🎁 Add kn-event configuration #154

Are you sure you want to change the base?

SRVCLI-401: 🎁 Add kn-event configuration #154

Conversation

cardil commented May 9, 2024 • edited Loading

Changes

openshift-ci bot commented May 9, 2024

openshift-ci-robot commented May 9, 2024

Changes

cardil commented May 9, 2024

cardil commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cardil Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented May 15, 2024

Changes

cardil commented Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented Jun 20, 2024

cardil commented May 9, 2024 •

edited

Loading

cardil Jun 18, 2024 •

edited

Loading

cardil commented Jun 19, 2024 •

edited

Loading