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

fix: PipelineLoop nested validation logic fixed. #638

Merged

Conversation

ScrapCodes
Copy link
Contributor

@ScrapCodes ScrapCodes commented Jun 25, 2021

Added missing enablement of feature-flag enable-custom-task.
Also added nested task validation.

Which issue is resolved by this Pull Request:
Resolves #637

Description of your changes:

Environment tested:

  • Python Version (use python --version):
  • Tekton Version (use tkn version):
  • Kubernetes Version (use kubectl version):
  • OS (e.g. from /etc/os-release):

Checklist:

It was missing enablement of feature-flag enable-custom-task.
Also added nested task validation.
@ScrapCodes ScrapCodes force-pushed the fix-pipelineloop-nested-validate branch from 586796e to ea3a47f Compare June 25, 2021 08:04
@ScrapCodes ScrapCodes changed the title fix: PipelineLoop nested validation logic fixed. [WIP] fix: PipelineLoop nested validation logic fixed. Jun 25, 2021
@ScrapCodes
Copy link
Contributor Author

I am getting a different error:

{"level":"error","ts":"2021-06-25T15:15:48.619Z","logger":"pipelineloop-controller","caller":"controller/controller.go:548","msg":"Reconcile error","knative.dev/controller":"github.com.kubeflow.kfp-tekton.tekton-catalog.pipeline-loops.pkg.reconciler.pipelinelooprun.Reconciler","knative.dev/kind":"tekton.dev.Run","duration":0.169647362,"error":"1 error occurred:\n\t* error creating PipelineRun from Run pr-loop-example-loop-task-qdgxr: admission webhook \"webhook.pipeline.tekton.dev\" denied the request: mutation failed: cannot decode incoming new object: json: unknown field \"spec\"\n\n","stacktrace":"knative.dev/pkg/controller.(*Impl).handleErr\n\tknative.dev/[email protected]/controller/controller.go:548\nknative.dev/pkg/controller.(*Impl).processNextWorkItem\n\tknative.dev/[email protected]/controller/controller.go:531\nknative.dev/pkg/controller.(*Impl).RunContext.func3\n\tknative.dev/[email protected]/controller/controller.go:468"}

@Tomcli
Copy link
Member

Tomcli commented Jun 25, 2021

@ScrapCodes I'm still getting this error from your example

{"level":"info","ts":"2021-06-25T17:07:14.058Z","logger":"pipelineloop-controller.event-broadcaster","caller":"record/event.go:282","msg":"Event(v1.ObjectReference{Kind:\"Run\", Namespace:\"default\", Name:\"pr-loop-example-loop-task-twnjn\", UID:\"81902555-3930-4f49-b339-55802d7dc877\", APIVersion:\"tekton.dev/v1alpha1\", ResourceVersion:\"42311078\", FieldPath:\"\"}): type: 'Warning' reason: 'Failed' PipelineLoop default/pr-loop-example-loop-task-twnjn can't be Run; it has an invalid spec: missing field(s): spec.pipelineSpec.tasks[1].taskSpec.steps"}

I think the error you have is due to some bad decoding in the inlined JSON? Let me also add few more debug messages to see can I find out the error.

@Tomcli
Copy link
Member

Tomcli commented Jun 25, 2021

I'm able to make nested loop working after applying your new context to the reconciler
ScrapCodes#2

If you can add a unit test for the enableCustomTaskFeatureFlag function and apply it to the reconciler, then this PR should be good to go.

@google-cla
Copy link

google-cla bot commented Jun 28, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

3 similar comments
@google-cla
Copy link

google-cla bot commented Jun 28, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Jun 28, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Jun 28, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@ScrapCodes ScrapCodes force-pushed the fix-pipelineloop-nested-validate branch from 98e73a4 to 6f09bb1 Compare June 28, 2021 10:22
@google-cla
Copy link

google-cla bot commented Jun 28, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@ScrapCodes ScrapCodes force-pushed the fix-pipelineloop-nested-validate branch from 6f09bb1 to 8e9e573 Compare June 28, 2021 10:35
@google-cla
Copy link

google-cla bot commented Jun 28, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@ScrapCodes ScrapCodes force-pushed the fix-pipelineloop-nested-validate branch from 8e9e573 to 457809f Compare June 28, 2021 12:30
@google-cla
Copy link

google-cla bot commented Jun 28, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@ScrapCodes ScrapCodes changed the title [WIP] fix: PipelineLoop nested validation logic fixed. fix: PipelineLoop nested validation logic fixed. Jun 28, 2021
@Tomcli
Copy link
Member

Tomcli commented Jun 28, 2021

@googlebot I consent.

@Tomcli
Copy link
Member

Tomcli commented Jun 28, 2021

I think some tests are broken when upgrading to Tekton 0.25, but the functionality for this PR is not effected. I will open another issue to fix the tests. For now I will merge this PR to move the inline spec forward.

@Tomcli
Copy link
Member

Tomcli commented Jun 28, 2021

/lgtm
/approve

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ScrapCodes, Tomcli

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

@google-oss-robot google-oss-robot merged commit 12acff8 into kubeflow:master Jun 28, 2021
@ScrapCodes ScrapCodes deleted the fix-pipelineloop-nested-validate branch September 3, 2021 09:05
ScrapCodes added a commit to ScrapCodes/kfp-tekton that referenced this pull request Oct 18, 2021
google-oss-robot pushed a commit that referenced this pull request Oct 20, 2021
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.

PipelineLoop controller cannot handle nested inline custom task
3 participants