openshift-pipelines / pipelines-as-code

Pipelines-as-Code for Tekton
https://pipelinesascode.com
Apache License 2.0
124 stars 81 forks source link

Refactor configutil and fix bug with catalog parsing #1703

Closed piyush-garg closed 3 weeks ago

piyush-garg commented 3 weeks ago

This will refactor configutil to pass the validators in sync config function so that defaulting and validation can be performed separately

default validators are exposed to be used by deps

catalog parsing was broken as soon as it find the other key for same catalog so fixed that

refactored internal catalog parsing to have index data in map to convert the strut back to configmap

added utility to convert struct back to configmap to be used by deps and added unit tests

Submitter Checklist

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 83.09859% with 12 lines in your changes missing coverage. Please review.

Project coverage is 64.67%. Comparing base (3f9bcd0) to head (c3136ec).

Files Patch % Lines
pkg/params/settings/convert.go 77.77% 7 Missing and 3 partials :warning:
pkg/cmd/tknpac/resolve/resolve.go 0.00% 1 Missing :warning:
pkg/params/info/info.go 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1703 +/- ## ========================================== + Coverage 64.59% 64.67% +0.07% ========================================== Files 144 145 +1 Lines 11151 11192 +41 ========================================== + Hits 7203 7238 +35 - Misses 3422 3426 +4 - Partials 526 528 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

chmouel commented 3 weeks ago

Cc @sm43

chmouel commented 3 weeks ago

/retest

piyush-garg commented 3 weeks ago

@chmouel Updated the PR

chmouel commented 3 weeks ago

what about adding a new custom catalog in there (we do already add the custom one so name it custom2 or something):

https://github.com/openshift-pipelines/pipelines-as-code/blob/3f9bcd01268c5f8d5424637964e7eb2481e836a3/hack/dev/kind/install.sh#L187

and add another annotation to use it after this line:

https://github.com/openshift-pipelines/pipelines-as-code/blob/3f9bcd01268c5f8d5424637964e7eb2481e836a3/test/testdata/pipelinerun_remote_task_annotations.yaml#L13

so we are testing multiple custm catalog ? (which is what this bug fixes isnt it)

piyush-garg commented 3 weeks ago

what about adding a new custom catalog in there (we do already add the custom one so name it custom2 or something):

https://github.com/openshift-pipelines/pipelines-as-code/blob/3f9bcd01268c5f8d5424637964e7eb2481e836a3/hack/dev/kind/install.sh#L187

and add another annotation to use it after this line:

https://github.com/openshift-pipelines/pipelines-as-code/blob/3f9bcd01268c5f8d5424637964e7eb2481e836a3/test/testdata/pipelinerun_remote_task_annotations.yaml#L13

so we are testing multiple custm catalog ? (which is what this bug fixes isnt it)

Yup make sense, adding that