STONEBLD-1844 Add controller for PipelineRuns #210

stuartwdouglas · 2023-11-03T01:52:57Z

This controller looks for build PipelineRuns and will record an event that a downstream component should be 'nudged'.

This is the first part of STONEBLD-1756.

main.go

go.mod

controllers/component_pipelinerun_nudge_controller.go

mmorhun · 2023-11-03T16:24:33Z

controllers/component_pipelinerun_nudge_controller.go

+	for i := range components.Items {
+		comp := components.Items[i]
+		if comp.Spec.Application == updatedComponent.Spec.Application && slices.Contains(updatedComponent.Spec.BuildNudgesRef, comp.Name) {
+			//TODO: Do the nudge


Nudge would be a complicated operation that takes some time and involves dealing with git provider API.
What if nudge for one of the components fails? What if it's transient/consistent error?

This story was just about getting it to the point where we can print the message in the logs. I assume we will need a retry count mechanism with back off, we don't want to retry indefinitely, but we also don't want a single transient failure to stop the update.

Probably a question for the future implementation, but how would we distinguish already nudged components from not nudged one in case if error?

We will need to add an annotation with the state information. Also nudging should be idempotent, if you run the same nudge again it should figure out that there is an existing PR, and the update to the PR would result in the same commit.

I have added support for this.

Something to bear in mind with error handling too is that the actual updates will be async, and error handling will also need to be done through watching the job/pipeline that is doing the update.

stuartwdouglas · 2023-11-04T00:00:25Z

@mmorhun I think I have addressed everything

controllers/component_dependency_update_controller.go

stuartwdouglas · 2023-11-06T07:35:49Z

Given that both @tkdchen and @mmorhun asked about failure handling I have added failure handling and the retry mechanism to this PR. Currently it won't really do anything as printing log messages won't fail, but there are unit tests for artificial failure.

controllers/component_dependency_update_controller.go

go.mod

controllers/component_dependency_update_controller.go

mmorhun · 2023-11-07T11:36:28Z

controllers/component_dependency_update_controller.go

+	ctrllog "sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+	"strings"


nitpick: probably "strings" should go to the "standard" block and the block moved to the beginning.

mmorhun · 2023-11-07T11:41:49Z

controllers/component_dependency_update_controller.go

+	alreadyProcessed := map[string]bool{}
+	if pipelineRun.Annotations != nil && pipelineRun.Annotations[AlreadyNudgedAnnotationName] != "" {
+		list := strings.Split(pipelineRun.Annotations[AlreadyNudgedAnnotationName], ",")
+		for _, i := range list {


using i as a string key is confusing

controllers/component_dependency_update_controller.go

mmorhun · 2023-11-07T12:08:58Z

controllers/component_dependency_update_controller.go

+				}
+				val = val + i
+			}
+			pipelineRun.Annotations[AlreadyNudgedAnnotationName] = val


What if a new build will start in meantime?

That is what I mentioned above about builds going stale. When I perform the 'nudge' I could check for currently running pipelines for the same component that are older than the current state, and if they exist and are still running mark them as already nudged and remove the finalizer.

The other approach would be that when a pipeline finishes we check if a newer pipeline exists and has finished, however I don't think that approach will work very well as the pruner may have removed already completed pipelines.

I have implemented the stale check logic

stuartwdouglas · 2023-12-08T15:03:02Z

/retest

stuartwdouglas · 2023-12-08T16:07:54Z

/retest

stuartwdouglas · 2023-12-11T23:36:24Z

/retest

controllers/component_dependency_update_controller.go

mmorhun · 2023-12-13T11:59:39Z

controllers/component_dependency_update_controller.go

+		if err != nil {
+			return ctrl.Result{}, err
+		}
+		log.Error(nudgeErr, fmt.Sprintf("component update of as a result of a build of %s failed", updatedComponent.Name), l.Audit, "true")


Should this log be moved before Get?

controllers/component_dependency_update_controller.go

controllers/renovate_util.go

mmorhun · 2023-12-13T15:46:44Z

controllers/pac_pipelinerun_pruner_controller.go

@@ -65,7 +65,7 @@ func (r *PaCPipelineRunPrunerReconciler) SetupWithManager(mgr ctrl.Manager) erro
 		Complete(r)
 }

-//+kubebuilder:rbac:groups=tekton.dev,resources=pipelineruns,verbs=get;list;watch;delete;deletecollection
+//+kubebuilder:rbac:groups=tekton.dev,resources=pipelineruns,verbs=get;list;watch;delete;deletecollection;update;patch


Can we avoid adding these permissions here?

Apparently not, I tried adding them to the new controller and the ones here took precedence. Seemed to be an issue with the controller gen.

pkg/git/github/github_app.go

chmeliik · 2023-12-13T15:50:25Z

I had some comments here before they got lost in the github history, not sure if you've had a chance to address them: #210 (comment) (and the following ones)

chmeliik · 2023-12-13T15:57:56Z

controllers/renovate_util.go

+func GetAllGithubInstallations(ctx context.Context, client client.Client, eventRecorder record.EventRecorder, componentList []v1alpha1.Component) (string, []installationStruct, error) {
+	log := log2.FromContext(ctx)
+	// Check if GitHub Application is used, if not then skip
+	pacSecret := v1.Secret{}
+	globalPaCSecretKey := types.NamespacedName{Namespace: buildServiceNamespaceName, Name: prepare.PipelinesAsCodeSecretName}


General review-ability observation: next time, it would be nice to split the PR into commits that make it easier to review. Specifically, some of the changes in this PR just move code from one place to another - it would have been nice to do that in a separate commit and explain that there's no change in functionality, that it's just moving code around. (I'm not actually sure that there are no changes in the code that got moved, just assuming that's true).

Having it all in a single commit makes it hard to focus on the things that matter, first one has to figure out what is or isn't a relevant change

This controller looks for build PipelineRuns and will record an event that a downstream component should be 'nudged'. The controller then uses renovate to perform the 'nudge' and updates the downstream repositories with the new hash.

sonarqubecloud · 2023-12-13T23:40:29Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

5 New issues
0 Security Hotspots
No data about Coverage
1.9% Duplication on New Code

See analysis details on SonarCloud

stuartwdouglas · 2023-12-14T00:40:23Z

/retest

stuartwdouglas · 2023-12-14T02:10:54Z

/retest

mmorhun · 2023-12-14T16:24:53Z

controllers/component_dependency_update_controller.go

+	index := strings.LastIndex(image, ":")
+	if index != -1 {


To not to have func wide variable index, you can do:

if index := strings.LastIndex(image, ":"); index != -1 {

openshift-ci bot requested review from mmorhun and psturc November 3, 2023 01:53

stuartwdouglas force-pushed the STONEBLD-1844 branch 3 times, most recently from e98daf6 to 6a7cb16 Compare November 3, 2023 05:10

mmorhun reviewed Nov 3, 2023

View reviewed changes

mmorhun assigned stuartwdouglas Nov 3, 2023

stuartwdouglas force-pushed the STONEBLD-1844 branch 2 times, most recently from db77258 to b835e5a Compare November 4, 2023 00:00

stuartwdouglas force-pushed the STONEBLD-1844 branch 3 times, most recently from 785a945 to ea5e6b5 Compare November 4, 2023 01:32

tkdchen reviewed Nov 6, 2023

View reviewed changes

controllers/component_dependency_update_controller.go Outdated Show resolved Hide resolved

stuartwdouglas force-pushed the STONEBLD-1844 branch from ea5e6b5 to 8683378 Compare November 6, 2023 06:11

tkdchen reviewed Nov 6, 2023

View reviewed changes

controllers/component_dependency_update_controller.go Outdated Show resolved Hide resolved

tkdchen reviewed Nov 6, 2023

View reviewed changes

controllers/component_dependency_update_controller.go Outdated Show resolved Hide resolved

mmorhun reviewed Nov 6, 2023

View reviewed changes

stuartwdouglas force-pushed the STONEBLD-1844 branch 4 times, most recently from 8d27a99 to b611547 Compare November 7, 2023 01:46

tkdchen reviewed Nov 7, 2023

View reviewed changes

controllers/component_dependency_update_controller.go Show resolved Hide resolved

tkdchen reviewed Nov 7, 2023

View reviewed changes

controllers/component_dependency_update_controller.go Outdated Show resolved Hide resolved

stuartwdouglas force-pushed the STONEBLD-1844 branch 2 times, most recently from 7e37caa to faae7fa Compare November 7, 2023 04:34

mmorhun reviewed Nov 7, 2023

View reviewed changes

stuartwdouglas force-pushed the STONEBLD-1844 branch 2 times, most recently from 132430a to daa0e1c Compare November 8, 2023 03:34

stuartwdouglas force-pushed the STONEBLD-1844 branch from 1006851 to c307dd6 Compare December 11, 2023 22:26

stuartwdouglas force-pushed the STONEBLD-1844 branch 11 times, most recently from 2487aab to 1822dc9 Compare December 13, 2023 01:22

mmorhun reviewed Dec 13, 2023

View reviewed changes

chmeliik reviewed Dec 13, 2023

View reviewed changes

stuartwdouglas force-pushed the STONEBLD-1844 branch 2 times, most recently from 8e2cb96 to f93c48d Compare December 13, 2023 22:47

STONEBLD-1844 Add controller for PipelineRuns

2e52d4d

This controller looks for build PipelineRuns and will record an event that a downstream component should be 'nudged'. The controller then uses renovate to perform the 'nudge' and updates the downstream repositories with the new hash.

stuartwdouglas force-pushed the STONEBLD-1844 branch from f93c48d to 2e52d4d Compare December 13, 2023 23:40

mmorhun approved these changes Dec 14, 2023

View reviewed changes

openshift-ci bot assigned mmorhun Dec 14, 2023

openshift-ci bot added the lgtm label Dec 14, 2023

stuartwdouglas merged commit b4956a3 into konflux-ci:main Dec 14, 2023
12 of 14 checks passed

rh-tap-build-team bot mentioned this pull request Dec 14, 2023

build-service update redhat-appstudio/infra-deployments#2916

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STONEBLD-1844 Add controller for PipelineRuns #210

STONEBLD-1844 Add controller for PipelineRuns #210

stuartwdouglas commented Nov 3, 2023

mmorhun Nov 3, 2023

stuartwdouglas Nov 4, 2023

mmorhun Nov 6, 2023

stuartwdouglas Nov 6, 2023

stuartwdouglas Nov 7, 2023

stuartwdouglas Nov 7, 2023

stuartwdouglas commented Nov 4, 2023

stuartwdouglas commented Nov 6, 2023

mmorhun Nov 7, 2023

mmorhun Nov 7, 2023

mmorhun Nov 7, 2023

stuartwdouglas Nov 8, 2023

stuartwdouglas Nov 8, 2023

stuartwdouglas commented Dec 8, 2023

stuartwdouglas commented Dec 8, 2023

stuartwdouglas commented Dec 11, 2023

mmorhun Dec 13, 2023

mmorhun Dec 13, 2023

stuartwdouglas Dec 13, 2023

chmeliik commented Dec 13, 2023

chmeliik Dec 13, 2023

sonarqubecloud bot commented Dec 13, 2023

stuartwdouglas commented Dec 14, 2023

stuartwdouglas commented Dec 14, 2023

mmorhun Dec 14, 2023

STONEBLD-1844 Add controller for PipelineRuns #210

STONEBLD-1844 Add controller for PipelineRuns #210

Conversation

stuartwdouglas commented Nov 3, 2023

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

stuartwdouglas commented Nov 4, 2023

stuartwdouglas commented Nov 6, 2023

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

stuartwdouglas commented Dec 8, 2023

stuartwdouglas commented Dec 8, 2023

stuartwdouglas commented Dec 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chmeliik commented Dec 13, 2023

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 13, 2023

Quality Gate passed

stuartwdouglas commented Dec 14, 2023

stuartwdouglas commented Dec 14, 2023

Choose a reason for hiding this comment