-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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(conformance): create v1.7 conformance test directory #7123
Conversation
Updated KFP test image to 1.7
@@ -0,0 +1,28 @@ | |||
apiVersion: apps/v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing copyright header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -0,0 +1,31 @@ | |||
apiVersion: apps/v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing copyright header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
conformance/1.7/README.md
Outdated
|
||
Before running the conformance tests, you need to configure kubectl default context to point to the k8s cluster that is hosting Kubeflow. | ||
|
||
TODO: Make the kubeflow namespace configurable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: move this out of README.md
. Makefile
, maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. The KF service namespace was made configurable.
TEST_PROFILE ?= kf-conformance-test | ||
KUBEFLOW_NAMESPACE ?= kubeflow | ||
|
||
setup: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Replace here and below:
.PHONY: setup
setup:
...
This will prevent the make <rule>
from failing when there is a <rule>
file inside the folder. For example, if someone removes the .yaml
extension from setup.yaml
, make setup
won't behave as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
**Run conformance test** | ||
|
||
`make run` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: add brief comments on how to run component-specific tests (e.g. make run-kfp
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
containers: | ||
- name: kfp-conformance | ||
# TODO: update the image version to 1.0 | ||
image: gcr.io/ml-pipeline/kfp-conformance:1.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Should we add a comment pointing to the source of this image and how to properly keep it in sync? Same applies to other component tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @james-jwu. We should probably add license headers to katib and training-operator tests. I also left a few NIT-picks.
/assign @zijianjoy |
/lgtm Thank you James for adding conformance tests! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@zijianjoy or @james-jwu need to approve
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gkcalat, james-jwu 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 |
Updated KFP test image tag to 1.7.