Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pipelinerun controller (draft, don't merge!) #114

Draft
wants to merge 16 commits into
base: refactoring-pipelinerun
Choose a base branch
from

Conversation

FernandesMF
Copy link
Contributor

Hi team, I finished a first draft of the new PipelineRun controller.

This controller has the responsibility of checking how many pipelineruns are running at the moment, and if there are fewer than a specified value, kick new pipelineruns until the maximum is reached.

Could you please review the code and give your suggestions? Thanks! 😁

ref: CWFHEALTH-3293

Signed-off-by: Giulia Naponiello <[email protected]>
@qixiang qixiang force-pushed the refactoring-pipelinerun branch from 1231021 to a8749f6 Compare December 9, 2024 08:03
Copy link
Contributor

@qixiang qixiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor comments.
Could you rebase it to latest refactoring-pipelinerun branch? which could make it much easier for reviewing.

if req.Namespace != MintMakerNamespaceName {
return ctrl.Result{}, nil
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can ignore PipelineRun updates that not related to status change, when a PipelineRun has status change to error, failed or succeed, but not sure which is the best approach to detect that, maybe status.completionTime?

internal/controller/pipelinerun_controller.go Outdated Show resolved Hide resolved
internal/controller/pipelinerun_controller.go Outdated Show resolved Hide resolved
var _ = Describe("Pod Controller", func() {
Context("When reconciling a resource", func() {

It("should successfully reconcile the resource", func() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can keep this for a second time, but let's also write some tests. I think it will be sufficient to test that when a PipelineRun finishes a new one starts.

@@ -0,0 +1,16 @@
apiVersion: tekton.dev/v1beta1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably this file doesn't need to be pushed. If needed we can create a directory such as utils (maybe with a better name) and we can include some useful files there.

@gnaponie
Copy link
Contributor

gnaponie commented Dec 9, 2024

I've also added only minor comments, the change overall looks good! I've checked only the pipelinerun controller, as I'm assuming the rest is from previous commits. But as Qixiang said, let's rebase the branch, also because my changes were merged, so now they are more correct.

@FernandesMF FernandesMF force-pushed the pipelinerun_controller branch from 25fba72 to 74b0cae Compare December 9, 2024 22:09
@FernandesMF
Copy link
Contributor Author

FernandesMF commented Dec 9, 2024

@gnaponie @qixiang thank you for your reviews, I really appreciate them!
I'll address all the points you raised. In the meantime, I have rebased on top of refactor-pipeline, please feel free to have another look if you want. (And sorry for not having rebased before... I just drafted the MR and didn't realise it was a almost 1k lines long 😅 )

@qixiang qixiang force-pushed the refactoring-pipelinerun branch from e3b71bd to d42eb96 Compare December 12, 2024 07:28
Copy link
Contributor

@qixiang qixiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rebase it on top of the latest "refactoring-pipelinerun` branch, so we can focus on the necessary changes?

@qixiang qixiang force-pushed the refactoring-pipelinerun branch from fa468a4 to 0587273 Compare January 16, 2025 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants