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.

Sign up for GitHub

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

Jump to bottom

[pipeline/trusted-resources] Verify Task and Pipeline for Trusted Task #886

Merged
merged 1 commit into from
Jun 6, 2022
Merged

[pipeline/trusted-resources] Verify Task and Pipeline for Trusted Task #886

merged 1 commit into from
Jun 6, 2022

Conversation

austinzhao-go
Copy link
Contributor

@austinzhao-go austinzhao-go commented May 31, 2022 •
edited
Loading

Changes

The related issue: #873

Add Task and Pipeline Verification in Trusted Task repo

Follow-up PR:
Refact/clean error on return vals: #888

TODO

  • Add tests for "Verify Task"
  • Add impl for "Verify Pipeline"

Submitter Checklist

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

  • Includes docs (if user facing) - Docs will follow with current user guides
  • Commit messages follow commit message best practices
  • Commit messages includes a project tag in the subject - e.g. [OCI], [hub], [results], [task-loops]

See the contribution guide
for more details.

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 31, 2022
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 31, 2022
@austinzhao-go austinzhao-go changed the title Verify Task Verify Task and Pipeline for Trusted Task May 31, 2022
@austinzhao-go austinzhao-go force-pushed the feature/verify-task-pipeline-when-applying branch from a352903 to d5f1922 Compare June 2, 2022 18:08
@austinzhao-go austinzhao-go marked this pull request as ready for review June 2, 2022 18:08
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 2, 2022
@austinzhao-go austinzhao-go force-pushed the feature/verify-task-pipeline-when-applying branch 2 times, most recently from b0dc170 to f34abab Compare June 3, 2022 13:42
@austinzhao-go austinzhao-go changed the title Verify Task and Pipeline for Trusted Task [trusted-task] Verify Task and Pipeline for Trusted Task Jun 3, 2022
@austinzhao-go austinzhao-go changed the title [trusted-task] Verify Task and Pipeline for Trusted Task [pipeline/trusted-resources] Verify Task and Pipeline for Trusted Task Jun 3, 2022
@austinzhao-go austinzhao-go force-pushed the feature/verify-task-pipeline-when-applying branch from f34abab to 59f7b5d Compare June 3, 2022 14:00
@austinzhao-go
Copy link
Contributor Author

/assign @Yongxuanzhang @wlynch

wlynch
wlynch requested changes Jun 6, 2022
View reviewed changes
Copy link
Member

@wlynch wlynch left a comment

Choose a reason for hiding this comment

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

Looks good for the most part! Just a few small things.

pipeline/trusted-resources/pkg/trustedtask/trustedpipeline.go Outdated Show resolved Hide resolved
pipeline/trusted-resources/pkg/trustedtask/trustedtask_test.go Show resolved Hide resolved
pipeline/trusted-resources/pkg/trustedtask/trustedtask.go Outdated Show resolved Hide resolved
@austinzhao-go
Verify Task and Pipeline
b94e00f 
As the part of Trusted Task project, this work will support verifying Task and Pipeline.
@austinzhao-go austinzhao-go force-pushed the feature/verify-task-pipeline-when-applying branch from 59f7b5d to b94e00f Compare June 6, 2022 17:06
@austinzhao-go austinzhao-go mentioned this pull request Jun 6, 2022
Merged
3 tasks
wlynch
wlynch approved these changes Jun 6, 2022
View reviewed changes
Copy link
Member

@wlynch wlynch left a comment

Choose a reason for hiding this comment

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

/lgtm

pipeline/trusted-resources/pkg/trustedtask/trustedtask.go
Comment on lines +23 to +32
func (t *TrustedTask) Validate(ctx context.Context) (errs *apis.FieldError) {
k8sClient := kubeclient.Get(ctx)
tektonClient := client.Get(ctx)

if err := t.verifyTask(ctx, k8sClient, tektonClient); err != nil {
return err
}

return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Non blocking for this PR, but this is pretty trivial now - it may make sense to collapse this down to 1 func.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks Billy. so here mean we could arrange verifyTask into Validate, so having a single func right?

pipeline/trusted-resources/pkg/trustedtask/trustedtask.go
Comment on lines +43 to +45
cfg := config.FromContextOrDefaults(ctx)
cfg.FeatureFlags.EnableTektonOCIBundles = true
ctx = config.ToContext(ctx, cfg)
Copy link
Member

Choose a reason for hiding this comment

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

Ideally it'd be great if we didn't need to inject this into the context, but if this is always going to be set for every type, consider setting this for the whole controller here -

experimental/pipeline/trusted-resources/cmd/webhook_trusted_resource/main.go

Line 65 in b94e00f

return store.ToContext(ctx)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't realize this. thanks this hint!

mark this as a follow up action. see if need certain flexibility or not here for following work. if always, will update in webhook.
cc @Yongxuanzhang

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 6, 2022
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wlynch

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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 6, 2022
@tekton-robot tekton-robot merged commit d074361 into tektoncd:main Jun 6, 2022
@austinzhao-go austinzhao-go mentioned this pull request Jun 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wlynch wlynch wlynch approved these changes

@Yongxuanzhang Yongxuanzhang Awaiting requested review from Yongxuanzhang

Assignees

@wlynch wlynch

@Yongxuanzhang Yongxuanzhang

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@austinzhao-go @tekton-robot @wlynch @Yongxuanzhang