openshift-pipelines / pipelines-as-code

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

Adds configutils package to validate and assign config #1643

Closed sm43 closed 3 months ago

sm43 commented 3 months ago

adds a package to replace code in settings for validation and defaults with a generic func, using json tag for defaulting. and for figuring out which field it is from configmap.

Changes

Submitter Checklist

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 93.97590% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 64.62%. Comparing base (3274759) to head (9d45d01).

:exclamation: Current head 9d45d01 differs from pull request most recent head 78fd105. Consider uploading reports for the commit 78fd105 to get more accurate results

Files Patch % Lines
pkg/configutil/config.go 85.07% 8 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1643 +/- ## ========================================== + Coverage 64.11% 64.62% +0.50% ========================================== Files 140 141 +1 Lines 10912 10961 +49 ========================================== + Hits 6996 7083 +87 + Misses 3382 3355 -27 + Partials 534 523 -11 ```

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

chmouel commented 3 months ago

looks good by me, thanks!

enarha commented 1 month ago

@sm43 Hi. We just paid attention the error-detection-simple-regexp regex "^(?P<filename>[^:]*):(?P<line>[0-9]+):(?P<column>[0-9]+):([ ]*)?(?P<error>.*)" slightly changed from the value we have here https://github.com/openshift-pipelines/pipelines-as-code/blob/main/config/302-pac-configmap.yaml#L91 Was that change intentional? If yes, what is its purpose?

sm43 commented 1 month ago

intentional

@enarha I think I kept it the same to what was in config.go earlier https://github.com/sm43/pipelines-as-code/blob/78fd10528890116b21d6d5caed50a56ad7096539/pkg/params/settings/config.go#L58

although I am not sure why it has been a little different than in configmap