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

chore: Add kfp-tekton integration tests and manifests #10702

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

rimolive
Copy link
Member

Description of your changes:
Solves #10489

Adds the kfp-tekton integration tests and corresponding manifests

Checklist:

@rimolive
Copy link
Member Author

/hold

Wait for tests to pass before ask for review/approval

Copy link
Member

@Tomcli Tomcli left a comment

Choose a reason for hiding this comment

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

few comments on the registry name.

@rimolive rimolive force-pushed the kfp-branch branch 6 times, most recently from d26397a to 587e602 Compare April 18, 2024 21:58
@rimolive
Copy link
Member Author

Test are failling because some resources don't have namespace explicitly set. Some examples:

I tried to find a way to set in the kustomization.yaml level to set namespace in the objects that don't have namespace set but no luck. That being said, I'd like to ask @Tomcli and @chensun :

  • Is it ok if I set namespace in these Deployments?
  • If not, do you suggest another way to make them deploy in kubeflow namespace without explicitly setting in these objects?

@rimolive rimolive force-pushed the kfp-branch branch 4 times, most recently from b9f1d7d to f7c638d Compare April 19, 2024 00:49
Copy link
Member

@Tomcli Tomcli left a comment

Choose a reason for hiding this comment

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

few comments on fixing the old kustomize code to kustomize 5.2.1

Copy link
Member

@Tomcli Tomcli left a comment

Choose a reason for hiding this comment

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

Also, I think you didn't copy some role permission from the persistent agent, so the end to end test wasn't able to update the run status.

https://github.com/kubeflow/kfp-tekton/blob/v2-integration/manifests/kustomize/base/pipeline/ml-pipeline-persistenceagent-role.yaml#L29-L45

@rimolive
Copy link
Member Author

Also, I think you didn't copy some role permission from the persistent agent, so the end to end test wasn't able to update the run status.

https://github.com/kubeflow/kfp-tekton/blob/v2-integration/manifests/kustomize/base/pipeline/ml-pipeline-persistenceagent-role.yaml#L29-L45

Thank you for that! I'm still trying to figure you why the flip coin pipeline never finishes. At least with that change I can see the pipeline status of RUNNING, but it stays in that status until failure.

@rimolive rimolive force-pushed the kfp-branch branch 10 times, most recently from 2764782 to 1501193 Compare April 20, 2024 22:36
@rimolive
Copy link
Member Author

Just a checkpoint comment: Tests are passing, but I still need to address Tommy's comment on Kustomize 5.2.1 support and the re-tag script.

Signed-off-by: Ricardo M. Oliveira <rmartine@redhat.com>
@rimolive
Copy link
Member Author

/unhold
cc @Tomcli @chensun

Copy link
Contributor

@DharmitD DharmitD left a comment

Choose a reason for hiding this comment

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

Skimmed through the integration test results in CI and verified they work as expected.

/lgtm

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

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

@rimolive
Copy link
Member Author

@chensun Looks like e2e test did not finish well, but unrelated to the PR. Should we try run again?

@rimolive
Copy link
Member Author

/test kubeflow-pipeline-e2e-test

@google-oss-prow google-oss-prow bot merged commit 114bee7 into kubeflow:master Apr 22, 2024
7 checks passed
juliusvonkohout added a commit to kubeflow/manifests that referenced this pull request May 2, 2024
See kubeflow/pipelines#10702

Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
google-oss-prow bot pushed a commit to kubeflow/manifests that referenced this pull request May 2, 2024
* Update kubeflow/pipelines manifests from 2.2.0

Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>

* Update sync-pipelines-manifests.sh

Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>

* Update kubeflow/pipelines manifests from 2.2.0

Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>

* Delete sync-kfp-tekton-manifests.sh

See kubeflow/pipelines#10702

Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>

---------

Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
doncorsean pushed a commit to doncorsean/kubeflow-manifests that referenced this pull request Jul 18, 2024
* Update kubeflow/pipelines manifests from 2.2.0

Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>

* Update sync-pipelines-manifests.sh

Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>

* Update kubeflow/pipelines manifests from 2.2.0

Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>

* Delete sync-kfp-tekton-manifests.sh

See kubeflow/pipelines#10702

Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>

---------

Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
@rimolive rimolive deleted the kfp-branch branch August 28, 2024 22:11
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