Support "git" resolver in build pipeline config

[p-kabelka]

The build service controller currently supports only pipelines fetched using the "bundles" resolver.
However the rpmbuild-pipeline (upstream flavor, fedora flavor and the internal one) uses the "git" resolver for its Tasks.

It would be very preferable if the "git" resolver would be natively supported as it would allow the pipeline flavors to be onboarded to Konflux without the need of implementing a process to build Task bundles and potentially allow other pipelines to be easily onboarded.

Is the build-service team willing to support other types of resolvers other than "bundles"?

[chmeliik]

Is the build-service team willing to support other types of resolvers other than "bundles"?

I don't see a reason not to. Are you looking to contribute or to have us implement the support?

I will also note:

However the rpmbuild-pipeline (upstream flavor, fedora flavor and the internal one) uses the "git" resolver for its Tasks.

That's not necessarily a problem. You only need an OCI bundle for the pipeline YAML itself. The Tasks in it can be referenced by git resolver and I don't think that will cause any problems.

[p-kabelka]

You only need an OCI bundle for the pipeline YAML itself. The Tasks in it can be referenced by git resolver

I see, that's good to know, because that might be acceptable solution.

And for:

Are you looking to contribute or to have us implement the support?

If the pipeline git resolver is a must have for the RPM build pipeline, I can contribute an implementation.

[p-kabelka]

Starting to work on this, can you give me advice on how to run tests for this project? I see it contains multiple testing frameworks must mainly ginkgo. Are there seperate commands to run tests or is running ginkgo enough?

And do you have advice on testing my implementation if I should write unit tests for it, because component_build_controller_pipeline.go doesn't have tests. Or at least write a test in component_build_controller_test.go to verify if it works correctly.

[mmorhun]

Just make test will run all unit and integration tests.
Majority of the tests are integration tests that are written using ginkgo, because many things are not easy to cover with unit tests.

[p-kabelka]

So far it looks to be much more complicated than I'd hoped.

1. I want to ask about the lines 158-188: there's a comment // requested pipeline was not found in configMap but that's true only if the pipeline name in annotation is not in config map and the bundle is latest, but if both name and bundle are not in the config map, then it should continue to resolve it but from my experience Konflux will show an error that it cannot configure PaC.
   Is this the desired behavior or is this code supposed to allow only pipelines specified in the config map?

2. If the git resolver is to be supported, how should the pipeline in config map be defined?
   There are many posibilities:

    pipelines:
    - name: docker-build
      bundle: quay.io/konflux-ci/tekton-catalog/pipeline-docker-build...
    - name: build-rpm-package
      git-url: https://github.com/konflux-ci/rpmbuild-pipeline.git
      git-revision: main
      git-path: pipeline/build-rpm-package.yaml

and the component annotation may look like this:

build.appstudio.openshift.io/pipeline: '{"name":"build-rpm-package","bundle":"latest"}'

or like this:

build.appstudio.openshift.io/pipeline: '{"name":"build-rpm-package","git":"latest"}'

or depending on my first point if anything should be enforced to match the config map, overrides could also be allowed (would use the pipeline defined as with "git":"latest" with different branch):

build.appstudio.openshift.io/pipeline: '{"name":"build-rpm-package","git-revision":"other-branch"}'

The implementation is still very much WIP, but I'd like to know your feedback if I'm at least going the right direction with the suggested parameters.

[p-kabelka]

I refined the suggested parameters a bit so that they are more in line the way bundles resolver works.
So the git resolver could work by requiring all three git resolver parameters but each of them would support the replacement value latest same as bundle parameter:

build.appstudio.openshift.io/pipeline: '{"name":"build-rpm-package","git-url":"latest","git-revision":"latest","git-path":"latest"}'

and any of them could be overridden same as the bundle parameter:

build.appstudio.openshift.io/pipeline: '{"name":"build-rpm-package","git-url":"latest","git-revision":"other-branch","git-path":"latest"}'

[mmorhun]

Sorry for the delay @p-kabelka

1. The comment you've mentioned must be under if statement. If we failed to find pipeline with specific name in the config map and bundle is latest we should fail.
2. I agree with the in config map definition of git resolver, but having latest for git values looks weird to me. In my opinion just providing name should be enough (and give the ability to override git-revision. I see no sense (but might be wrong) in replacing just git repo url or path within the repo. If user wants to define all three, then there is no point in using the config map values at all: leave name empty? ).
3. You might need to fix SetDefaultBuildPipelineComponentAnnotation as well.

I think that the logic with bundles can be reworked the same way: having only name to be mandatory in case of using the config map (we require both due to historical reasons when we had bundles with tags instead of digests). @rcerven WDYT?

[p-kabelka]

I wanted to quickly provide some initial suggestion to start a discussion about a potential solution, but I'll be able to get back to this probably after new year.

In my opinion just providing name should be enough (and give the ability to override git-revision. I see no sense (but might be wrong) in replacing just git repo url or path within the repo. If user wants to define all three, then there is no point in using the config map values at all: leave name empty? ).

I kept the name there to be consistent with bundles, but it can probably be omitted since it doesn't rely on it.

You might need to fix SetDefaultBuildPipelineComponentAnnotation as well.

In case the default gets switched to a git resolver instead of bundles resolver right?

[rcerven]

Sorry for the delay @p-kabelka

1. The comment you've mentioned must be under `if` statement. If we failed to find pipeline with specific name in the config map and bundle is `latest` we should fail.

2. I agree with the in config map definition of git resolver, but having `latest` for git values looks weird to me. In my opinion just providing name should be enough (and give the ability to override `git-revision`. I see no sense (but might be wrong) in replacing just git repo url or path within the repo. If user wants to define all three, then there is no point in using the config map values at all: leave name empty? ).

3. You might need to fix `SetDefaultBuildPipelineComponentAnnotation` as well.

I think that the logic with bundles can be reworked the same way: having only name to be mandatory in case of using the config map (we require both due to historical reasons when we had bundles with tags instead of digests). @rcerven WDYT?

yeah having just pipeline name mandatory is fine, as in that case it will just get bundle from configMap for the specific pipeline

[p-kabelka]

While writing tests for the implementation, I came up with a few additional requirements.

1. Since the main supported content type are containers, the generated PipelineRun always contains dockerfile parameter which is unnecessary for RPMs and other possible content types.
2. The parameter output-image is also always generated and is also unused by some pipelines, for example the rpmbuild-pipeline.
3. I want to ask about the usage of pipelineSpec in the generated PipelineRun, can pipelineRef be used instead, at least for content types other than containers? My second question is why was it chosen in the first place instead of pipelineRef, since the generated PipelineRun still contains references to task bundles and not the whole pipeline, it seems to me that a reference could be used at the pipeline level instead of task level.

These changes for content types other than containers would need a way to differentiate what to generate, could a new annotation/parameter in Component CR be added to specify the content type what the component represents?
Something similar to:

Example

metadata:
  annotations:
    content-type: rpm

metadata:
  annotations:
    content-type: container

And would default to container if missing or empty.

[p-kabelka]

I feel like an annotation may just move the problem with hardcoded parameters and not solve it. Maybe a better way would be to extend the additional-params parameter in ConfigMap which currently adds the listed parameters with their default values from the selected pipeline.
Additional params could be extended to allow referencing values from the Component CR for example like this:

apiVersion: v1
kind: ConfigMap
metadata:
  name: build-pipeline-config
  namespace: build-service
data:
  config.yaml: |
    default-pipeline-name: docker-build-oci-ta
    pipelines:
    - name: fbc-builder
      bundle: quay.io/konflux-ci/tekton-catalog/pipeline-fbc-builder@sha256:b7a3d170793e284b4ee77e3a77314d0bb9f2b548bbd5e88f64b91a0d29c36f77
    - name: docker-build
      bundle: quay.io/konflux-ci/tekton-catalog/pipeline-docker-build@sha256:18dddcb75d34f68338a1f17a5f27614018e6dfe03df544908148a8d0f611f9c4
      additional-params:
      - name: dockerfile
        value-ref: /spec/source/git/dockerfileUrl
    - name: build-rpm-package
      git-url: https://github.com/konflux-ci/rpmbuild-pipeline.git
      git-revision: main
      git-path: pipeline/build-rpm-package.yaml
      additional-params:
      - name: package-name
      - name: target-branch
      - name: ociStorage
        value-ref: output-image

This would allow to make dockerfile parameter not mandatory for pipelines that do not use it. Pipelines that do use it would add it as additional parameter with path reference to the value in Component.

In the example above value-ref: output-image would mean to add a parameter ociStorage with the value that would be generated as output-image, which means the output-image parameter would not need to be added to all the pipelines in ConfigMap.

What do you think about these extensions?

[p-kabelka]

I switched to using the synchronous git resolver from Tekton instead of the async resolver and I wanted to ask if that's a valid solution because it runs regular git commands in the background, so git needs to be installed where the build-service is running.

[mmorhun]

1 .Since the main supported content type are containers, the generated PipelineRun always contains dockerfile parameter which is unnecessary for RPMs and other possible content types.

2. The parameter output-image is also always generated and is also unused by some pipelines, for example the rpmbuild-pipeline.

We can improve it and add such parameters only if specified bundle contains the parameter.

I want to ask about the usage of pipelineSpec in the generated PipelineRun, can pipelineRef be used instead

I think yes, we can use pipeline ref instead of the whole definition.

why was it chosen in the first place instead of pipelineRef

In short, due to historical reasons, originally we wanted to have everything customizable by users, but later on some users requested just reference to have concise pipeline definition (and due to other reasons).

Maybe a better way would be to extend the additional-params parameter in ConfigMap

To me it looks somewhat redundant, we can figure it out from pipeline definition when the Operator fetches the bundle. It's the same as for items 1-2 at the beginning.

it runs regular git commands in the background, so git needs to be installed

I'm not sure about git in the image... Why don't we just keep the git resolver in pipeline run and let PaC / Tekton do the job?