openshift-pipelines / pipeline-service

SaaS for Tekton Pipelines
Apache License 2.0
23 stars 44 forks source link

PLNSRVCE-1625: disable beta api feature fields and enable alpha field… #937

Closed jkhelil closed 6 months ago

jkhelil commented 6 months ago

konflux users want to be able to set resources at step level in the pipeline definition. Acutally using computeResources at PipelineTaskRunSpec is not inconvenient, because the setting per task gets divided per step. Users want to have fined grained resources configurations at step level. Using stepSpecs (refre to the doc and code ) is covering the user requirement, but it needs to set feature fiels to alpha for tektonconfig. As far as i tested, I am not seen any regression du this new setting.

https://github.com/tektoncd/pipeline/blob/018c4b84bf850d4cc1565863dab281ad07abc069/pkg/apis/pipeline/v1/pipelinerun_types.go#L637 https://tekton.dev/docs/pipelines/pipeline-api/#tekton.dev/v1.PipelineTaskRunSpec

for the record here is a manifest for testing the feature


kind: PipelineRun
metadata:
  name: hello-34
spec:
  taskRunSpecs:
    - pipelineTaskName: hello
      stepSpecs:
      - name: hello
        computeResources:
          limits:
            cpu: 100m
            memory: 100Mi
          requests:
              cpu: 50m
              memory: 50Mi
  pipelineSpec:
    tasks:
      - name: hello
        taskSpec:
          steps:
            - name: hello
              image: ubuntu
              script: "echo hello world!"`
jkhelil commented 6 months ago

@Roming22 @enarha @avinal PTAL

gabemontero commented 6 months ago

my opinion @Roming22 @enarha @jkhelil this warrants creating a test branch in infra-deployments that points to a branch with this change

also, as you know @Roming22 there is a bit of a history wrt this setting.... for some reason the slack thread link in the jira is not working for me, so I can't see the discussion there, but minimally I'd be curious on who weighed in on this decision.

Perhaps worth discussion in the huddle with the pipelines team next week? Or did they already weigh in?

enarha commented 6 months ago

What I recall from the previous discussion on the matter: 1) alpha features are not supported (so it'll be up to us to fill that gap) 2) alpha features can be dropped (hard to take back functionality from the user) 3) alpha features are all or nothing (can't be limited to a single feature). I wonder if the step in question can't be replaced by a separate task with that step?

gabemontero commented 6 months ago

to summarize our discussion between @Roming22 @enarha @jkhelil and myself wrt enabling alpha, the summary is we are OK for now with enabling alpha as long as the support contract for users is understood, and if there are problems, there may not be resolution and they have to back off use of the alpha feature

on testing, @Roming22 was going to interface with @jkhelil to show how we craft infra-deployment PRs that reference non-main branches on pipeline-service to run the e2e's

gabemontero commented 6 months ago

/ok-to-test

Roming22 commented 6 months ago

@jkhelil Opened the PR on infra-deployment: https://github.com/redhat-appstudio/infra-deployments/pull/3257

jkhelil commented 6 months ago

Test PR on infra-deployments is green

jkhelil commented 6 months ago

/retest

jkhelil commented 6 months ago

/test Red Hat Konflux / test-pipeline-service-deployment-ocp-414

jkhelil commented 6 months ago

/test test-pipeline-service-deployment-ocp-414