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(tests): remove redundant integration test wf #11322

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

droctothorpe
Copy link
Contributor

Description of your changes:
Identical sample tests are being run in two separate CI workflows for some reason: kfp-samples and kubeflow-pipelines-integration-v2. The first invokes sample_test.py. The second does as well (after pathing through a Makefile). Presumably, we don’t need both.

I suggest we tackle eliminating integration-test.sh and some of the corresponding out of date docs in a follow-up PR. There should be one, clearly documented path for running large tests both in CI and from local.

Checklist:

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.

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Oct 21, 2024
@github-actions github-actions bot added the ci-passed All CI tests on a pull request have passed label Oct 21, 2024
@droctothorpe
Copy link
Contributor Author

/ok-to-test

@VaniHaripriya
Copy link
Contributor

/lgtm

Copy link
Contributor

@hbelmiro hbelmiro left a comment

Choose a reason for hiding this comment

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

I suggest we tackle eliminating integration-test.sh

@droctothorpe can you remove the file in this PR.

and some of the corresponding out of date docs

FYI, We have an open issue for that: #11061

@google-oss-prow google-oss-prow bot removed the lgtm label Oct 22, 2024
Signed-off-by: droctothorpe <[email protected]>
@github-actions github-actions bot removed the ci-passed All CI tests on a pull request have passed label Oct 22, 2024
Copy link
Contributor

@hbelmiro hbelmiro left a comment

Choose a reason for hiding this comment

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

Thank you for addressing my suggestions @droctothorpe.

I suggest we tackle eliminating integration-test.sh

@droctothorpe can you remove the file in this PR.

You probably missed this comment. Can we remove the file in this PR if it doesn't impact anything?

@droctothorpe
Copy link
Contributor Author

Thank you for addressing my suggestions @droctothorpe.

I suggest we tackle eliminating integration-test.sh

@droctothorpe can you remove the file in this PR.

You probably missed this comment. Can we remove the file in this PR if it doesn't impact anything?

Done! 👍

Signed-off-by: droctothorpe <[email protected]>
@github-actions github-actions bot added the ci-passed All CI tests on a pull request have passed label Oct 22, 2024
@rimolive
Copy link
Member

Based on @hbelmiro suggestions that was already addressed, I believe we're good to go.

/lgtm

Copy link
Contributor

@hbelmiro hbelmiro left a comment

Choose a reason for hiding this comment

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

Thank you @droctothorpe.
/lgtm

@HumairAK
Copy link
Collaborator

/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HumairAK

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-prow google-oss-prow bot merged commit 6a35ee5 into kubeflow:master Oct 22, 2024
16 checks passed
@chensun
Copy link
Member

chensun commented Oct 23, 2024

Just to share a bit history, in the past, sample tests and integration tests are different workflows, IIRC, the former runs against the latest release--we do this by maintaining a long running deployment and update it post every release--while the latter runs against the built from master head + the PR being tested--developing on the fly.
I sense this current GA workflow may have merged, but in the long run, I think there're value to test with both stable (release) and branch head.

@droctothorpe droctothorpe deleted the fix-ci branch October 23, 2024 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved ci-passed All CI tests on a pull request have passed lgtm ok-to-test size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants