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

Change sample test to read test arguments from yaml config file #1987

Merged
merged 15 commits into from
Aug 29, 2019

Conversation

numerology
Copy link

@numerology numerology commented Aug 29, 2019

Part of #1750
Now the params used for each sample test live in test/sample-test/configs dir. If no config file is provided, default param values will be used. Also, common constants used in sample test infra are moved into a dedicated python file.

Moving image injection and into config files guideline of adding a sample test will be in following PRs.


This change is Reviewable

@Ark-kun Ark-kun changed the title [WIP] Change sample test to digest parameters from yaml config file [WIP] Change sample test to read test arguments from yaml config file Aug 29, 2019
@numerology numerology changed the title [WIP] Change sample test to read test arguments from yaml config file Change sample test to read test arguments from yaml config file Aug 29, 2019
@gaoning777
Copy link
Contributor

Could we add a readme to explain the steps to add a sample test?

@numerology
Copy link
Author

Could we add a readme to explain the steps to add a sample test?

Sure thing. Will do that in another PR.

@Ark-kun
Copy link
Contributor

Ark-kun commented Aug 29, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 29, 2019
@gaoning777
Copy link
Contributor

gaoning777 commented Aug 29, 2019

Could we add a readme to explain the steps to add a sample test?

Sure thing. Will do that in another PR.

If not the readme, could you provide the spec about the config yaml in this PR? I think this should be a minimal work such as

testname:
    type: string
    required: true
test_timeout:
    type: float
    required: false
arguments:
    type: object
    required: false

This will be important because users need to know what the sample test infra is expecting from the configuration.
WDYT?

@numerology
Copy link
Author

Could we add a readme to explain the steps to add a sample test?

Sure thing. Will do that in another PR.

If not the readme, could you provide the spec about the config yaml in this PR? I think this should be a minimal work such as

testname:
    type: string
    required: true
test_timeout:
    type: float
    required: false
arguments:
    type: object
    required: false

This will be important because users need to know what the sample test infra is expecting from the configuration.
WDYT?

Just to confirm. Are you talking about add this spec in a separated file, or inline in the comment in the config.yaml itself?

@Ark-kun
Copy link
Contributor

Ark-kun commented Aug 29, 2019

/lgtm

@gaoning777
Copy link
Contributor

/lgtm

@numerology
Copy link
Author

/test kubeflow-pipeline-sample-test

@numerology
Copy link
Author

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: numerology

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

@numerology numerology merged commit 4918d19 into kubeflow:master Aug 29, 2019
@numerology numerology deleted the change-to-yaml-spec-sample-test branch August 29, 2019 23:30
magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
…#1987)

* update copyright info in openapi

Signed-off-by: Suresh Nakkeran <suresh.n@ideas2it.com>

* updated copyright info

Signed-off-by: Suresh Nakkeran <suresh.n@ideas2it.com>
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