Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: revert removal of lighthouse common pipelines & fix Tekton validation errors #1556

Merged

Conversation

Skisocks
Copy link
Contributor

@Skisocks Skisocks commented Jun 28, 2023

This PR reverts the changes made to remove lighthouse's Tekton pipeline parsing & fixes the issues seen with Tekton validating pipeline results values.

Issues with Tekton validation were due to new validation being added to the Pipeline.Results.Value object that checks the value is populated and correctly references a task within the pipelineSpec.

Added an info message that gets commented on PRs when opened if the pipelines in that repo use the uses: syntax.

@jenkins-x-bot
Copy link
Contributor

Hi @Skisocks. Thanks for your PR.

I'm waiting for a jenkins-x 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 jenkins-x/lighthouse repository.

@Skisocks
Copy link
Contributor Author

/cc @msvticket @tomhobson

@Skisocks
Copy link
Contributor Author

/hold

Holding for multiple reviews

@msvticket
Copy link
Member

/ok-to-test

@msvticket
Copy link
Member

Issues with Tekton validation were due to new validation being added to the Pipeline.Results.Value object that checks the value is populated and correctly references a task within the pipelineSpec.

So it is this problem that is fixed?

  Warning  Failed            51m   PipelineRun  Pipeline jx/ga12-gke-gsm-dev-pr-1-verify-v4hdl can't be Run; it contains Tasks that don't exist: Couldn't retrieve Task "": error requesting remote resource: error getting "Git" "jx/git-0ed44de9af3fdfd292e37feaab97f83b": revision error: reference not found
  Warning  InternalError     51m   PipelineRun  1 error occurred:
           * Couldn't retrieve Task "": error requesting remote resource: error getting "Git" "jx/git-0ed44de9af3fdfd292e37feaab97f83b": revision error: reference not found

jenkins-x/jx3-versions#3643 (comment)

@Skisocks
Copy link
Contributor Author

@msvticket No. That was a separate issue that was a result of going to native Tekton (a problem with the migration of pipeline-catalog). With reverting back to lighthouse's resolver then that error won't be relevant anymore.

The problem that b7b8816 fixes is an issue internal to lighthouse that we noticed when we first upgraded Tekton within lighthouse. Basically Tekton is doing way more validation on things now and is checking that the PipelineResults.Value object exists and actually points to an existing task. Previously lighthouse didn't populate this, so the results object was completely broken.

Copy link
Member

@tomhobson tomhobson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get a test with both a git ref and a uses step to make sure that they don't work together?

@Skisocks
Copy link
Contributor Author

Skisocks commented Jul 7, 2023

Example message:
image

@Skisocks
Copy link
Contributor Author

Skisocks commented Jul 7, 2023

@tomhobson @msvticket Not sure about mentioning the issue though as it might get mentioned a tonne by jx pipelines.
image

@msvticket
Copy link
Member

@tomhobson @msvticket Not sure about mentioning the issue though as it might get mentioned a tonne by jx pipelines. image

If you don't have access to the repo you won't see the message. (I don't see this.) And since most active repositories are bound to be private I don't think it's a problem as such. But I don't think we should add the comment before we actually have a suggested fix.

Copy link
Member

@tomhobson tomhobson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to move forward as I think the messaging can tee up the changes we're going to make quite nicely.

We have users that aren't in slack that don't know this will be a future change. it would be nice to let them know before it happens.

@jenkins-x-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msvticket, tomhobson

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 [msvticket,tomhobson]

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

@Skisocks
Copy link
Contributor Author

/hold cancel

@jenkins-x-bot
Copy link
Contributor

Failed to merge this PR due to:

failed merging [1556]: [Method Not Allowed]

@jenkins-x-bot jenkins-x-bot merged commit 048aadf into jenkins-x:main Jul 11, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants