Move merging of workflow section into webhook #168

kstrenkova · 2024-08-12T18:34:28Z

Currently, the merging of workflow section is done in controllers of related CRs. We want to move the merging logic into webhooks to make the process cleaner.

Depends on: #160

openshift-ci · 2024-08-12T18:34:32Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

lpiwowar

@kstrenkova, this is really nice progress! 🎉 I say we schedule a call and discuss details but generally speaking I think we can make this work. I've just tried to test this locally. I had to make some minor tweaks (see comments) for the merging to work on the TempestRun section on my machine.

Anyway, really good job! 👍 Even when you do not manage to finish this task and somebody will be working on it instead of you then you still laid a really good foundation here!

From our last conversation I was under the impression that this won't work. I think this will still require 2 - 3 PR updates and reviews but it is on a good path.

api/v1beta1/tempest_webhook.go

controllers/tempest_controller.go

kstrenkova · 2024-10-22T12:48:22Z

I have added a new version of this patch that can also work with structures by calling the function for adding values from TempestRun section recursively. There is maybe needed a bit more generalization to get the functions working for all resources, but for Tempest this version should work.

lpiwowar

I think the PR is moving in the right direction. Nice work! 👍

I've added couple of comments @kstrenkova. Let me know what you think:).

api/v1beta1/common_webhook.go

controllers/common.go

api/v1beta1/common_webhook.go

softwarefactory-project-zuul · 2024-12-03T09:59:58Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/openstack-k8s-operators/test-operator for 168,acf536c9054f5be35f0179a5d34578f8977aa19c

openshift-ci · 2025-01-06T13:22:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kstrenkova
Once this PR has been reviewed and has the lgtm label, please ask for approval from lpiwowar. For more information see the 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

softwarefactory-project-zuul · 2025-01-06T13:30:52Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/e601c4b6bb7844e9a3b8941fb5ccf4b6

❌ openstack-k8s-operators-content-provider FAILURE in 7m 44s
⚠️ podified-multinode-edpm-deployment-crc-test-operator SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

lpiwowar

Just a couple of questions:). Overall moving in a good direction.

lpiwowar · 2025-02-03T09:01:33Z

controllers/tobiko_controller.go

 	// NOTE(lpiwowar): Move all the merge code to the webhook once it is completed.
 	//                 It will clean up the workflow code and remove the duplicit code
 	//                 (Tempest vs Tobiko)
-	if step < len(instance.Spec.Workflow) {
-		if instance.Spec.Workflow[step].NodeSelector != nil {
-			instance.Spec.NodeSelector = *instance.Spec.Workflow[step].NodeSelector
+	if workflowStepNum < len(instance.Spec.Workflow) {
+		if instance.Spec.Workflow[workflowStepNum].NodeSelector != nil {
+			instance.Spec.NodeSelector = *instance.Spec.Workflow[workflowStepNum].NodeSelector
 		}

-		if instance.Spec.Workflow[step].Tolerations != nil {
-			instance.Spec.Tolerations = *instance.Spec.Workflow[step].Tolerations
+		if instance.Spec.Workflow[workflowStepNum].Tolerations != nil {
+			instance.Spec.Tolerations = *instance.Spec.Workflow[workflowStepNum].Tolerations
 		}

-		if instance.Spec.Workflow[step].SELinuxLevel != nil {
-			instance.Spec.SELinuxLevel = *instance.Spec.Workflow[step].SELinuxLevel
+		if instance.Spec.Workflow[workflowStepNum].SELinuxLevel != nil {
+			instance.Spec.SELinuxLevel = *instance.Spec.Workflow[workflowStepNum].SELinuxLevel
 		}

-		if instance.Spec.Workflow[step].Resources != nil {
-			instance.Spec.Resources = *instance.Spec.Workflow[step].Resources
+		if instance.Spec.Workflow[workflowStepNum].Resources != nil {
+			instance.Spec.Resources = *instance.Spec.Workflow[workflowStepNum].Resources
 		}
 	}


question: Can we remove this code?

If the implementation works correctly then in the end this code should be removed altogether.

nit (non-blocking): Also, try to avoid renaming of variables unless absolutely necessary when doing bigger PRs or at least make it a separate commit. This makes the review a little bit more complicated as renaming of step to workflowStepNum is not 100 % connected to this PR.

I am planning on removing this code, but first I want the patch to work. I also do not know how this works precisely, so I left it there for now.

I renamed the variable because everywhere it was workflowStepNum except here. But I can split the commits when it's done, or if it's not too difficult then even now.

Ok, good:). I'm keeping this comment open so that we do not forget about this.

Regarding the second point. Try to think about a PR as story. You're trying to make it as easy as possible for the reviewer. By splitting the PR into multiple commits it is much more easier to see the chunks of code that are coupled together (each commit is like a chapter). But no rush with this. It just caught my attention when going through the code.

lpiwowar · 2025-02-03T09:08:14Z

controllers/common.go

+	}
+}
+
+func mergeSections(main interface{}, workflow interface{}) {


question: Can you remind me. Why do we need this function when we are performing the merging into the workflow in the hook?

So in my understanding, if we want to get for example tempestRun and there is no workflow we take value from the main section. If there is a workflow section, we can't just take from the workflow spec because the Types are different there than for main section. If we want it to work in both scenarios, we need to set it like main section, so it gets correct types.

But I also don't like the duplicate logic in this. Is there a way to make the types more compatible than doing it this way 🤔

Ok, now I remember. Do you think we could maybe make the types the same both in the main section and in the workflow section? This way we could theoretically get rid of the function and it could simplify some things. But it would require at least testing it out:). I'm leaving it up to you.

It is a possibility, but I am not sure if that's not too big of a change 🤔 If you remember, why is the Workflow not the same but pointers? And sometimes not even pointers.. Maybe there is a reason why they should be pointers and then the function is a good idea.

The function will not be too difficult to have, if Workflow section structures stay as they are, but I do agree that it would look nicer without it. Less work for controller as well, everything would be in webhook. So all in all, I am down for both options. However, I am not sure what impact would changing types have. If it was not a problem, I would assume it would be done like that already.

It is a possibility, but I am not sure if that's not too big of a change 🤔 If you remember, why is the Workflow not the same but pointers?

The reason why we use pointers in the workflow section is because we need to be sure whether a value in the workflow section was set by a user or not. If the value is not set by the user then it is simply a nil. If we do not use pointers than the the values are set to a default value e.g for string "".

It might be a big change but if it works then I believe in this case it would be justified.

Again, I'm leaving this up to your judgment. But I believe if we can get rid of the function it would be better.

That's tricky. It sounds like it has a good reason to be as pointers then. But maybe the justification can be done 🤔 I also want to get rid of the function, but don't want to change how things are done too much.. As a person who was in charge of test-operator, would you say removing the pointer would be possible without big repercussions? If yes, I can go with the new strategy and push the changes for you to review.

It is not that tricky as it might appear at first glance. As a person who was in charge of test-operator my answer is that it is at least worth trying. All you have to do is to just:

set the types in the main section to pointers (here)

If you are using VS Code for example it is going to scream at you at all the places where the types are mismatched. You fix those issues.

You run test-operator and investigate whether it works or not.

If yes, I can go with the new strategy and push the changes for you to review.

Yes, but first please check that the code works as expected on your local environment.

I also want to get rid of the function, but don't want to change how things are done too much..

The main point of this PR is to simplify the code. Do not be afraid of changing the stuff. It is ok:)

I didn't expect you to suggest setting the main section to pointers. I though the change would be in the workflow section. Then it would be difficult to get when a parameter is empty 😄 Which was my concern, but you managed to reply earlier.

I think it would be possible to take the route of pointers, but I don't think it would look that great and it is changing code on a lot of places. I think the current function does a lot of code simplifying already. The duplicate code among CR types will be gone and it's only needed to call the function once 🤔 Should I still go for the types.go changes? I am still unsure.

Yes, I will check that the code works for the final review request :D I am just trying to get the structure with you right now.

Should I still go for the types.go changes?

As stated in my previous comment I believe it is worth trying. We can talk about it at the office on Thursday and updated the thread here after:).

I don't think it would look that great

Why?

api/v1beta1/common_webhook.go

softwarefactory-project-zuul · 2025-02-03T19:59:20Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/bcfbc898629c4820850aa6536ff2e0a2

✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 17m 08s
❌ podified-multinode-edpm-deployment-crc-test-operator FAILURE in 2h 06m 12s

Currently, the merging of workflow section is done in controllers of related CRs. We want to move the merging logic into webhooks to make the process cleaner.

softwarefactory-project-zuul · 2025-02-04T19:41:39Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/c5249a2766ce4d8f8dc4447597cb6532

✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 32m 59s
❌ podified-multinode-edpm-deployment-crc-test-operator FAILURE in 2h 07m 59s

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

openshift-ci bot added the approved label Aug 12, 2024

lpiwowar added the do-not-merge/hold label Aug 15, 2024

lpiwowar assigned kstrenkova Sep 19, 2024

lpiwowar self-requested a review September 19, 2024 14:57

lpiwowar mentioned this pull request Sep 24, 2024

Embed common structs #200

Merged

lpiwowar reviewed Sep 24, 2024

View reviewed changes

kstrenkova force-pushed the merge-workflow-section-in-webhook branch from e880a74 to af17dde Compare September 30, 2024 19:24

kstrenkova force-pushed the merge-workflow-section-in-webhook branch from af17dde to 6b1c12c Compare October 22, 2024 12:43

lpiwowar requested changes Oct 23, 2024

View reviewed changes

openshift-ci bot assigned lpiwowar Oct 23, 2024

openshift-ci bot removed the approved label Dec 3, 2024

openshift-merge-robot added the needs-rebase label Dec 3, 2024

kstrenkova force-pushed the merge-workflow-section-in-webhook branch from acf536c to 5160a26 Compare January 6, 2025 13:22

openshift-merge-robot removed the needs-rebase label Jan 6, 2025

kstrenkova force-pushed the merge-workflow-section-in-webhook branch from 5160a26 to 0287c9a Compare January 8, 2025 11:50

kstrenkova force-pushed the merge-workflow-section-in-webhook branch 3 times, most recently from 43f39c7 to 0ac4bba Compare January 30, 2025 15:57

lpiwowar reviewed Feb 3, 2025

View reviewed changes

kstrenkova force-pushed the merge-workflow-section-in-webhook branch from 0ac4bba to 9843799 Compare February 3, 2025 15:52

kstrenkova force-pushed the merge-workflow-section-in-webhook branch from 9843799 to 031df6c Compare February 4, 2025 14:10

Move merging of workflow section into webhook

94c4df1

Currently, the merging of workflow section is done in controllers of related CRs. We want to move the merging logic into webhooks to make the process cleaner.

kstrenkova force-pushed the merge-workflow-section-in-webhook branch from 031df6c to 94c4df1 Compare February 4, 2025 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move merging of workflow section into webhook #168

Move merging of workflow section into webhook #168

kstrenkova commented Aug 12, 2024

openshift-ci bot commented Aug 12, 2024

lpiwowar left a comment

kstrenkova commented Oct 22, 2024

lpiwowar left a comment

softwarefactory-project-zuul bot commented Dec 3, 2024

openshift-ci bot commented Jan 6, 2025

softwarefactory-project-zuul bot commented Jan 6, 2025

lpiwowar left a comment

lpiwowar Feb 3, 2025

kstrenkova Feb 3, 2025

lpiwowar Feb 4, 2025

lpiwowar Feb 3, 2025

kstrenkova Feb 3, 2025

lpiwowar Feb 4, 2025

kstrenkova Feb 4, 2025

lpiwowar Feb 4, 2025

kstrenkova Feb 4, 2025

lpiwowar Feb 4, 2025

kstrenkova Feb 4, 2025

lpiwowar Feb 5, 2025

softwarefactory-project-zuul bot commented Feb 3, 2025

softwarefactory-project-zuul bot commented Feb 4, 2025

Move merging of workflow section into webhook #168

Are you sure you want to change the base?

Move merging of workflow section into webhook #168

Conversation

kstrenkova commented Aug 12, 2024

openshift-ci bot commented Aug 12, 2024

lpiwowar left a comment

Choose a reason for hiding this comment

kstrenkova commented Oct 22, 2024

lpiwowar left a comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Dec 3, 2024

openshift-ci bot commented Jan 6, 2025

softwarefactory-project-zuul bot commented Jan 6, 2025

lpiwowar 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

softwarefactory-project-zuul bot commented Feb 3, 2025

softwarefactory-project-zuul bot commented Feb 4, 2025