Shipwright Triggers API #1008

otaviof · 2022-03-09T09:09:26Z

Changes

Adding the API for Shipwright Triggers (EP) with validation and initial documentation.

Submitter Checklist

Includes tests if functionality changed/was added
Includes docs if changes are user-facing
Set a kind label on this PR
Release notes block has been filled in, or marked NONE

Release Notes

Adding the API for Shipwright Triggers, a event driven approach to instantiate new builds

lookis · 2022-06-03T15:50:43Z

Hi @otaviof , thanks for your works, any update on this PR?

otaviof · 2022-06-09T17:14:21Z

/retitle Shipwright Triggers API

otaviof · 2022-06-09T17:15:10Z

/assign @SaschaSchwarze0 @adambkaplan

/cc @coreydaley

coreydaley · 2022-06-09T18:08:03Z

@otaviof I don't see the use of the trigger.when, why not just have triggers? Do you expect to add additional adverbs in the future? I would like some clarification on that before commenting on the specifics of the pull request since a lot of my thoughts pertain to that.

otaviof · 2022-06-10T04:14:31Z

Hi @otaviof , thanks for your works, any update on this PR?

Hello! Sorry for the delay, we are moving forward with this feature again 👍 And thanks for the heads up too.

otaviof · 2022-06-10T04:19:48Z

@otaviof I don't see the use of the trigger.when, why not just have triggers? Do you expect to add additional adverbs in the future? I would like some clarification on that before commenting on the specifics of the pull request since a lot of my thoughts pertain to that.

I'm implementing the changes we've discussed during the EP review without changes, so that's why we have the nested attribute approach.

Personally, the trigger.when sounds straightforward to explain the concept behind the API, thus a contender to .spec.triggers could be .spec.triggerWhen.

SaschaSchwarze0

I suggest to add validation to https://github.com/shipwright-io/build/blob/v0.10.0/pkg/validate/validate.go#L83-L119. Basically a BuildRun with an inline spec must not have triggers set.

docs/build.md

SaschaSchwarze0 · 2022-06-20T13:18:27Z

docs/build.md

+
+The GitHub type is meant to react on events coming from GitHub WebHook, the events are compared against the existing `Build` resources and therefore it can find `Build` based on `.spec.source.url` combined with the attributes on `.spec.trigger.when[].github`.
+
+When `.spec.trigger.when[].github.branches` is empty, the `.spec.source.revision` is employed, the default "branch" name employed is based on the regular source repository configuration.


I suggest to have a full defined list of all possible combinations to leave no room for speculation:

If the source contains a revision that is a branch name, and the trigger branches are empty, then BuildRuns get submitted for this single branch only.

If the source contains no revision and the trigger branches are empty, then ... TBD (how do we behave here, is that valid at all ?)

If the source contains a revision, and the trigger branches are not empty, then BuildRuns get submitted for the branches defined in the trigger.

If the source contains no revision, and the trigger branches are not empty, then BuildRuns get submitted for the branches defined in the trigger.

Let me start with an example, please. Let's imagine a pull-request is open at the repository X against the main branch, it will dispatch a GitHub Event which contains detailed

Okay, so let me start with a use-case example, please. Let's imagine the user is receiving WebHook events from GitHub, a event comes along with the type PullRequest listing the target branch as main and as well the repository URL. Based on those, it must then trigger a single BuildRun per Build object that matches both repository URL and revision, so in other words, there's a direct relationship between one GitHub event and one (or more) Build instance.

Given a single GitHub Event must trigger a single BuildRun per Build instance responsible for that repository, the business rule is basically a fallback approach: probe if the event branch name is listed on .spec.trigger.when[].github.branches slice, or, uses the .spec.source.revision (by default is assumed "main" when empty).

SaschaSchwarze0 · 2022-06-20T13:20:19Z

docs/build.md

+        type: Image
+        image:
+          names:
+            - ghcr.io/some/base-image:latest


Should not here be the name of the Image resource that must be in the same namespace ? Especially if the image is not at all in an external registry, it would be complicated to provide such an image name.

Here we are looking for a fully qualified image name and to later on rely on the image controller to handle this business logic. However, the image name is a free form string initially.

/cc @ricardomaraschini

SaschaSchwarze0 · 2022-06-20T13:22:23Z

docs/build.md

+          status:
+            - Succeeded


I may need to check the ship again, why do we filter on the status here? Should not it be always on successful completion?

In order to have a flexible workflow we should allow any state to be filled in, and it's expected that "Succeeded" is the most common.

Flexibility is fine, but out of interest, what would be the use case to trigger a BuildRun on a Pipeline status change other than to Succeeded?

The "status" is reported as a string, and the .objectRef can match more than one type of Kubernetes CRD.

Just for my understanding, does Pipeline CRD has a state, or is more about PipelineRun states?

Sorry, I haven't been clear before. The object that carries the state is the PipelineRun.

SaschaSchwarze0 · 2022-06-20T13:22:39Z

docs/build.md

+            - Succeeded
+          name: tekton-pipeline-name
+```
+


What about the BuildRun trigger? In my own subjective opinion, that is more important than pipelines.

The triggers is a mechanism to instantiate BuildRun objects based on a Build, so BuildRun would not be able to trigger itself.

Yep, I meant Build trigger. But I just notice it is not part of the SHIP, then it is fine for now. It is being discussed here: shipwright-io/community#75 (comment).

SaschaSchwarze0 · 2022-06-20T13:23:40Z

pkg/apis/build/v1alpha1/build_types.go

+	// SpecTriggersNameCanNotBeBlank indicates the when condition does not have a name
+	SpecTriggersNameCanNotBeBlank BuildReason = "SpecTriggersWhenNameCanNotBeBlank"
+	// SpecTriggersInvalidType indicates the when type is invalid
+	SpecTriggersInvalidType BuildReason = "SpecTriggersWhenInvalidType"
+	// SpecTriggersInvalidGitHub indicates the when type GitHub is invalid
+	SpecTriggersInvalidGitHub BuildReason = "SpecTriggersWhenInvalidGitHub"
+	// SpecTriggersInvalidImage indicates the when type Image is invalid
+	SpecTriggersInvalidImage BuildReason = "SpecTriggersWhenInvalidImage"
+	// SpecTriggersInvalidPipeline indicates the when type Pipeline is invalid
+	SpecTriggersInvalidPipeline BuildReason = "SpecTriggersWhenInvalidPipeline"


For consistency, I suggest to remove the Spec prefix everywhere. Those error codes are also documented on https://github.com/shipwright-io/build/blob/main/docs/build.md#build-validations. And I just notice that we forgot to add the Volume-related reasons.

SaschaSchwarze0 · 2022-06-20T13:26:24Z

pkg/apis/build/v1alpha1/trigger_when.go

+	Name string `json:"name"`
+
+	// Type the event type, the name of the webhook event.
+	Type WhenTypeName `json:"type,omitempty"`


If this is required, should we remove omitempty ?

Thanks, indeed the omitempty must not be there.

pkg/apis/build/v1alpha1/trigger_when.go

pkg/apis/build/v1alpha1/trigger_types.go

lookis · 2022-06-21T03:41:13Z

I have a question, If we watch github event, how to apply carried information to Build result?
for example, I have tag a commit in github repo, and trigger the Build. how to apply the tag to the output Image

SaschaSchwarze0 · 2022-06-21T06:38:10Z

I have a question, If we watch github event, how to apply carried information to Build result? for example, I have tag a commit in github repo, and trigger the Build. how to apply the tag to the output Image

Hi @lookis, qq, are you:

pushing a tag to your GitHub repository and expect a build to start for this tag ? This is not covered of the Triggers SHIP, but would be an interesting scenario @otaviof.
interested in getting the tag name or commit sha that is built somewhere into the image (as a label or annotation), or as its image tag ? Or as annotation of an image signature ? That's all future.

lookis · 2022-06-21T06:55:17Z

@SaschaSchwarze0 Thanks, looking forward to it.

openshift-ci · 2022-06-21T09:44:44Z

@otaviof: GitHub didn't allow me to request PR reviews from the following users: ricardomaraschini.

Note that only shipwright-io members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @ricardomaraschini

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/test-infra repository.

otaviof · 2022-06-21T09:49:28Z

I suggest to add validation to https://github.com/shipwright-io/build/blob/v0.10.0/pkg/validate/validate.go#L83-L119. Basically a BuildRun with an inline spec must not have triggers set.

Thanks for the review, please consider the latest changes.

otaviof · 2022-06-21T13:06:54Z

I have a question, If we watch github event, how to apply carried information to Build result? for example, I have tag a commit in github repo, and trigger the Build. how to apply the tag to the output Image

Hi @lookis, qq, are you:
* pushing a tag to your GitHub repository and expect a build to start for this tag ? This is not covered of the [Triggers SHIP](https://github.com/shipwright-io/community/blob/main/ships/0031-shipwright-trigger.md), but would be an interesting scenario @otaviof.

* interested in getting the tag name or commit sha that is built somewhere into the image (as a label or annotation), or as its image tag ? Or as annotation of an image signature ? That's all future.

Most definitely an interesting use-case, @lookis! I think we've spoken about having automation to tag the image accordingly with the context, like in this case the repository metadata. I think we should keep this in the radar.

Lets bring this on the next community meeting?

SaschaSchwarze0

/approve

qu1queee

@otaviof thank you, neat neat work!

Some minor comments, pls take a look

qu1queee · 2022-06-29T20:04:12Z

docs/build.md

+          status:
+            - Succeeded


Just for my understanding, does Pipeline CRD has a state, or is more about PipelineRun states?

pkg/apis/build/v1alpha1/trigger_when.go

pkg/validate/trigger.go

pkg/validate/validate.go

Co-authored-by: Enrique Encalada <encaladaenrique@gmail.com>

Co-authored-by: Sascha Schwarze <schwarzs@de.ibm.com>

qu1queee · 2022-07-09T07:02:52Z

@otaviof ty, approving.

/approve

openshift-ci · 2022-07-09T07:03:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qu1queee, SaschaSchwarze0

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~~ [SaschaSchwarze0,qu1queee]

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

qu1queee · 2022-07-09T07:03:03Z

/lgtm

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 9, 2022

openshift-ci bot requested review from qu1queee and SaschaSchwarze0 March 9, 2022 09:09

otaviof force-pushed the shipwright-trigger-api branch 2 times, most recently from bab9210 to ebf188d Compare March 10, 2022 15:03

otaviof mentioned this pull request Mar 15, 2022

SHIP-0031: Shipwright Trigger shipwright-io/community#68

Merged

gabemontero mentioned this pull request Mar 22, 2022

trigger BuildRun when source is updated #1017

Closed

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2022

otaviof force-pushed the shipwright-trigger-api branch from 17c8188 to 15304e2 Compare June 9, 2022 17:05

openshift-ci bot changed the title ~~[WIP] Shipwright-Triggers API~~ Shipwright Triggers API Jun 9, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 9, 2022

openshift-ci bot assigned adambkaplan and SaschaSchwarze0 Jun 9, 2022

openshift-ci bot requested a review from coreydaley June 9, 2022 17:15

otaviof changed the title ~~Shipwright Triggers API~~ Shipwright-Triggers API Jun 10, 2022

otaviof changed the title ~~Shipwright-Triggers API~~ Shipwright Triggers API Jun 10, 2022

otaviof added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 10, 2022

otaviof mentioned this pull request Jun 20, 2022

June 20th, 2022 Community Meeting shipwright-io/community#92

Closed

SaschaSchwarze0 requested changes Jun 20, 2022

View reviewed changes

otaviof force-pushed the shipwright-trigger-api branch from 54d4af1 to 7082c9e Compare June 21, 2022 09:43

otaviof requested a review from SaschaSchwarze0 June 21, 2022 09:49

SaschaSchwarze0 approved these changes Jun 22, 2022

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 22, 2022

qu1queee requested changes Jul 1, 2022

View reviewed changes

SaschaSchwarze0 mentioned this pull request Jul 4, 2022

July 4th, 2022 Community Meeting shipwright-io/community#94

Closed

otaviof force-pushed the shipwright-trigger-api branch from eccb60a to cdaf481 Compare July 7, 2022 08:49

otaviof and others added 3 commits July 7, 2022 14:10

Trigger API

763ec21

Co-authored-by: Enrique Encalada <encaladaenrique@gmail.com>

Generated Code and Kubernetes CRD

0ca9a26

Trigger API Documentation

3f6ef66

Co-authored-by: Sascha Schwarze <schwarzs@de.ibm.com>

otaviof force-pushed the shipwright-trigger-api branch from cdaf481 to 3f6ef66 Compare July 7, 2022 12:10

otaviof requested a review from qu1queee July 7, 2022 13:12

openshift-ci bot assigned qu1queee Jul 9, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2022

openshift-ci bot merged commit a44ef99 into shipwright-io:main Jul 9, 2022

SaschaSchwarze0 added this to the release-v0.11.0 milestone Aug 15, 2022


		The GitHub type is meant to react on events coming from GitHub WebHook, the events are compared against the existing `Build` resources and therefore it can find `Build` based on `.spec.source.url` combined with the attributes on `.spec.trigger.when[].github`.

		When `.spec.trigger.when[].github.branches` is empty, the `.spec.source.revision` is employed, the default "branch" name employed is based on the regular source repository configuration.

Shipwright Triggers API #1008

Shipwright Triggers API #1008

Conversation

otaviof commented Mar 9, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

lookis commented Jun 3, 2022

otaviof commented Jun 9, 2022

otaviof commented Jun 9, 2022

coreydaley commented Jun 9, 2022 • edited Loading

otaviof commented Jun 10, 2022

otaviof commented Jun 10, 2022

SaschaSchwarze0 left a 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

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

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

lookis commented Jun 21, 2022

SaschaSchwarze0 commented Jun 21, 2022

lookis commented Jun 21, 2022

openshift-ci bot commented Jun 21, 2022

otaviof commented Jun 21, 2022

otaviof commented Jun 21, 2022

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

qu1queee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qu1queee commented Jul 9, 2022

openshift-ci bot commented Jul 9, 2022

qu1queee commented Jul 9, 2022

otaviof commented Mar 9, 2022 •

edited

Loading

coreydaley commented Jun 9, 2022 •

edited

Loading