Remove Results Field for PipelineRuns and TaskRuns

[danielhelfand]

Closes #1243

This pull request removes the Results field for pipelineruns and taskruns from the Tekton pipeline API.

I also touched up some language in api_compatibility_policy.md after noticing some typos when reading it over.

Changes

Removes all Results *Results references throughout the project.

Submitter Checklist

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

• Includes tests (if functionality changed/added)
• Includes docs (if user facing)
• Commit messages follow commit message best practices

Reviewer Notes

If API changes
are included, additive changes
must be approved by at least two OWNERS
and backwards incompatible changes
must be approved by more than 50% of the OWNERS,
and they must first be added
in a backwards compatible way.

Release Notes

Removes Results field from API for PipelineRuns and TaskRuns

[tekton-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: danielhelfand
To complete the pull request process, please assign bobcatfish
You can assign the PR to them by writing /assign @bobcatfish in a comment when ready.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[tekton-robot]

Hi @danielhelfand. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[vdemeester]

/ok-to-test

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go	92.9%	100.0%	7.1
pkg/apis/pipeline/v1alpha1/taskrun_validation.go	91.3%	95.3%	4.0
pkg/reconciler/pipelinerun/pipelinerun.go	82.2%	82.1%	-0.1

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go	92.9%	100.0%	7.1
pkg/apis/pipeline/v1alpha1/taskrun_validation.go	91.3%	95.3%	4.0
pkg/reconciler/pipelinerun/pipelinerun.go	82.2%	82.1%	-0.1

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go	92.9%	100.0%	7.1
pkg/apis/pipeline/v1alpha1/taskrun_validation.go	91.3%	95.3%	4.0
pkg/reconciler/pipelinerun/pipelinerun.go	82.2%	82.1%	-0.1

[danielhelfand]

Addressed build/test issues. Should be ready for a review. Thanks!

[vdemeester]

/lgtm

[ghost]

Is this a backwards compatible change? Or has it already been placed into deprecated state for some number of releases?

Removal of fields has tripped us up a little bit before - thinking of the "Trigger" field that got removed from TaskRuns at the same time as the validation webhook started rejecting resources with unknown fields.

[danielhelfand]

@sbwsg I am unaware if the field has been deprecated and if there has been notice given, but this will break users who are using the Results field and some notice should be given.

According to your policy, it looks like you like to give a full release notice. I saw the issue was targeted for 0.7.0.

[danielhelfand]

However, judging by the writing in the issue, it seems as if this field doesn't really have much of a purpose other than to be a placeholder. Digging through the code that I removed in this pr, it doesn't appear like Results was really used for anything. So, unless I am mistaken, I think it's reasonable to assume this is not being used by any consumer of the API.

Update To Original Comment

The fact that this can be set, in general, is a breaking change and it's not fair to assume this is not used. Any guidance on how to best introduce this gracefully would be appreciated.

There's also the matter on what to do with result_types.go and its tests, as it appears it was created to supplement Results. Its validateURL method is used by pipelineresource so it can't be outright deleted.

[ghost]

I wonder if a good approach here might be to log a warning to indicate that the field is deprecated as of 0.7.0, document it as part of the 0.7.0 release notes, and then remove it completely in 0.8.0? I'm not sure if the warning part is possible though - @vdemeester @bobcatfish any thoughts on this?

[dibyom]

Yeah, we can definitely call it out in the release notes, and maybe log a warning in the controller logs though I'm not sure how useful that will be.

[danielhelfand]

I can agree with the idea that logging, at least in the ways we could, would probably not be the best way to communicate this. What if we announced the 0.7.0 release notes this field will be deprecated and also add in code comments to Results for both TaskRunSpec and PipelineRunSpec that these fields will be deprecated in 0.8.0?

[ghost]

sounds good to me

[danielhelfand]

@sbwsg Thanks for your help. I'll close this now and we can revisit it in 0.8.0. I can open a pr with comments announcing the deprecation in 0.8.0.