ArtifactsPipelineRunEnableDeepInspection making this field to accept both bool and string

[rupalibehera]

Changes

https://issues.redhat.com/browse/SRVKP-4387

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Run make test lint before submitting a PR
• Includes tests (if functionality changed/added)
• Includes docs (if user facing)
• Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

[tekton-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[rupalibehera]

@PuneetPunamiya does this make sense ?

[rupalibehera]

I have tested this with upgrading and it is working fine.

[jkandasa]

@rupalibehera can you please rebase 2 commits to one?

[PuneetPunamiya]

Thanks 🤙🏻
/lgtm

[piyush-garg]

@rupalibehera @PuneetPunamiya can you please give detail about why this change in required in commit message, and also is it required for all the three fields?

[rupalibehera]

@rupalibehera can you please rebase 2 commits to one?

done

[piyush-garg]

Also can we add some unit tests for this?

[rupalibehera]

@rupalibehera @PuneetPunamiya can you please give detail about why this change in required in commit message, and also is it required for all the three fields?

some boolean fields in tekton config for chains were string and some were boolean, while adding those it was like a guess work if it should be bool or string hence I have updated it, it was required only for one field https://issues.redhat.com/browse/SRVKP-4387 but I added for all others as well

[rupalibehera]

Also can we add some unit tests for this?

we do not have any existing unit tests for the same

[piyush-garg]

@rupalibehera @PuneetPunamiya can you please give detail about why this change in required in commit message, and also is it required for all the three fields?

some boolean fields in tekton config for chains were string and some were boolean, while adding those it was like a guess work if it should be bool or string hence I have updated it, it was required only for one field https://issues.redhat.com/browse/SRVKP-4387 but I added for all others as well

AFAIK, we do this when a field which was supposed to be boolean before, now is string, so to support upgrade, which change it like this to support both type/values. If this is the case here, please do this for the field which has got this change. Also, we should add validation defaulting etc accordingly. Also, good to have unit tests if you can add.

If this is not the case, and there is some other use case for this, can you please give details for that. Also, would be great if you can put all those details in commit msg and PR

[jkandasa]

@rupalibehera Thanks for the PR
Can you please add validation code on the BoolValue fields? That will be helpful to allow only the supported strings.
As we changed the bool to string type, now the input possibilities are limitless (earlier it was only true and false), So good to have validation code and unit test for the validation code.

Also please update the description about this PR, and include JIRA and/or GH issues references on the description, if we have any. and release notes to the users, as this is PR changes the user input.
This will be useful, when we look this PR in the future. All the required information will be in this PR.

[rupalibehera]

@piyush-garg @jkandasa I am just changing ArtifactsPipelineRunEnableDeepInspection field now and dropped the other fields.

[rupalibehera]

@piyush-garg @jkandasa , can we please merge this without the unit tests, there are very minimal changes now

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonchain_validation.go	71.7%	72.7%	1.0

[piyush-garg]

/approve

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jkandasa, piyush-garg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [jkandasa,piyush-garg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonchain_validation.go	71.7%	72.7%	1.0

[PuneetPunamiya]

/lgtm

[piyush-garg]

/cherry-pick release-v0.71.x

[tekton-robot]

@piyush-garg: once the present PR merges, I will cherry-pick it on top of release-v0.71.x in a new PR and assign it to you.

In response to this:

/cherry-pick release-v0.71.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[tekton-robot]

@piyush-garg: new pull request created: #2231

In response to this:

/cherry-pick release-v0.71.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.