-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Allow creating runs without experiments #1175
Allow creating runs without experiments #1175
Conversation
/test kubeflow-pipeline-e2e-test |
2 similar comments
/test kubeflow-pipeline-e2e-test |
/test kubeflow-pipeline-e2e-test |
bc6072c
to
f8fd216
Compare
6e9dd6d
to
52365d8
Compare
/test kubeflow-pipeline-e2e-test |
/test kubeflow-pipeline-e2e-test |
@@ -77,11 +78,11 @@ class AllRunsList extends Page<{}, AllRunsListState> { | |||
private _selectionChanged(selectedIds: string[]): void { | |||
const toolbarActions = [...this.props.toolbarProps.actions]; | |||
// Compare runs button | |||
toolbarActions[1].disabled = selectedIds.length <= 1 || selectedIds.length > 10; |
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.
The design of this toolbarActions array is a source of potential problems, I kind of regret doing it this way. I'd recommend adding a TODO to refactor it into an object with tighter type semantics.
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.
Agreed. Added a TODO
}, waitTimeout); | ||
}); | ||
|
||
it('displays both custom and default experiment experiment list page', () => { |
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.
extra "experiment"
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
it('displays both custom and default experiment experiment list page', () => { | ||
$('.tableRow').waitForVisible(); | ||
const rows = $$('.tableRow').length; | ||
assert(rows === 2, 'there should now be two experiments in the table, instead there are: ' + rows); |
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.
I'm curious, does the default experiment disappear if this experiment-less run goes away? What if it's archived for example?
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.
Nope. Experiments can exist without runs
/lgtm overall, just a few comments. |
5bf7a86
to
79cc1be
Compare
3f0d243
to
70c67dd
Compare
…ails due to the default experiment being deleted at the end of the API initialization and integration test suites
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yebrahim 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 |
Following #1089, this PR adds
Create run
button to Experiment/All run list pages.Also updates the frontend integration test to include creating a run without selecting an experiment, and filtering the experiment list.
This change is