Add experiment selector to NewRun #486
Conversation
84a7c51 to
55b6504
Compare
frontend/src/pages/NewRun.tsx
Outdated
| </div> | ||
| )} | ||
| <div> | ||
| <div>This run will be associated with the following experiment</div> |
There was a problem hiding this comment.
Should we change this message? Maybe "Select this run's experiment"?
There was a problem hiding this comment.
Maybe the description should be removed entirely? We don't say anything specific for the other fields
There was a problem hiding this comment.
I'd vote to keep it, since the UI still doesn't explain what experiments are anywhere, it's likely the least understood concept on that form.
There was a problem hiding this comment.
If that's the case, then I think we should keep the original wording as it's more explanatory
|
/test kubeflow-pipeline-e2e-test |
|
|
||
| it('displays resource selector', async () => { | ||
| const props = generateProps(); | ||
| props.columns = selectorColumns; |
There was a problem hiding this comment.
These three lines are redundant, no?
There was a problem hiding this comment.
Here yeah. It made sense when the resources were still explicitly referenced. Removed below as well, except for the resource itself so one can see how it changes
frontend/src/pages/NewRun.tsx
Outdated
| columns={this.experimentSelectorColumns} | ||
| emptyMessage='No experiments found. Create an experiment and then try again.' | ||
| initialSortColumn={ExperimentSortKeys.CREATED_AT} | ||
| resourceToRow={this._resourceToRow} |
There was a problem hiding this comment.
If both components use the same method, why not integrate it for now, until we need to separate it out?
f3839c6 to
c74616f
Compare
c74616f to
ff17bf3
Compare
90d951b to
5cea0cd
Compare
|
[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 |
|
[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 |
* Adds an experiment selector to the new run page. Needs tests * Adds an experiment selector to the new run page. Needs tests * Adds tests for the new experiment selector in NewRun * Rename PipelineSelector -> ResourceSelector since it handles experiments as well * Makes ResourceSelector more abstract. No longer coupled to experiments and pipelines * PR comments, NewRun clean-up * Moves resourceToRow function into ResourceSelector * Fix e2e test
* code complete * reduce forwarding rule max age * use operation resource * fix resource() * use max proxy age * move down cleanup_certificate * add check for pending targetHTTPproxy ops * update how to get domain * fix bug * update get_ssl_certificate_domain()
This is the first part of fixing #53 and #256
This change is