Add new run button to pipeline details #507
Conversation
|
/area front-end |
c670d21 to
098c3d6
Compare
0326bbd to
8dc9655
Compare
frontend/src/pages/NewRun.test.tsx
Outdated
|
|
||
| it('indicates that a pipeline is preselected and provides a means of selecting a different pipeline', async () => { | ||
| const props = generateProps(); | ||
| props.location.search = `?${QUERY_PARAMS.fromRunId}=${MOCK_RUN_WITH_EMBEDDED_PIPELINE.run!.id}`; |
There was a problem hiding this comment.
It would be nice if these two lines move to the describe level, make the code more readable IMO.
There was a problem hiding this comment.
Done. Reused the MOCK_RUN_WITH_EMBEDDED_PIPELINE so I reset it and the other mock objects before each test
frontend/src/pages/NewRun.test.tsx
Outdated
| })); | ||
| }); | ||
|
|
||
| it('displays a page error if ', async () => { |
There was a problem hiding this comment.
"... if referenced run has no embedded pipeline"
| @@ -322,6 +331,34 @@ class NewRun extends Page<{}, NewRunState> { | |||
| await this.showPageError(`Error: failed to retrieve original run: ${originalRunId}.`, err); | |||
| logger.error(`Failed to retrieve original run: ${originalRunId}`, err); | |||
| } | |||
| } else if(embeddedPipelineRunId) { | |||
There was a problem hiding this comment.
nit: Does it make sense for this logic to move into the _prepareFormFromClone? (we can rename that method accordingly, maybe _prepareFormFromRun).
Alternatively, we can add a new method, to keep this part of the code about branching conditions rather than branch implementations.
There was a problem hiding this comment.
Yes, I think that makes a lot of sense. Created a new function: _prepareFormFromEmbeddedPipeline.
Very little logic is shared with _prepareFormFromClone, so it makes sense to keep them separate.
| try { | ||
| templateString = JsYaml.safeDump(pipelineSpec); | ||
| } catch (err) { | ||
| await this.showPageError('Failed to convert pipeline spec to YAML.', err); |
There was a problem hiding this comment.
Maybe we shouldn't expose that level of detail? The user likely doesn't know anything about both the JSON and YAML formats of the pipeline at this point. Maybe just "Failed to parse pipeline spec."? So maybe it can just be joined with the below catch.
There was a problem hiding this comment.
I think it's good to still keep the logger statements separate so we can debug it easier, but yeah, the user-facing messages don't need to be different here.
frontend/src/pages/NewRun.tsx
Outdated
| pipelineFromRun: pipeline, | ||
| pipelineName: (pipeline && pipeline.name) || '', | ||
| usePipelineFromRun: true, | ||
| usePipelineFromRunLabel: 'Use pipeline from previous step', |
There was a problem hiding this comment.
Probably run is better than step here... Also "previous" seems a bit confusing.
Alternatively, we could have a pre-filled pipeline selector form field with a button to replace it with another pipeline.
I'm not sure the scenario where the user clones the run, but wants to change the pipeline, is useful. In that case nothing will be taken from the clone.
There was a problem hiding this comment.
page might make more sense than either. This text covers the case where a user goes to the pipeline details page of a pipeline that only exists inside of a run (it's not uploaded), so saying run doesn't really make sense.
This scenario is not about cloning, it's about creating a new run with a pipeline preselected. I agree that switching the pipeline after preselecting it or cloning is an strange path, but I don't want to make restricting assumptions without hearing @ajayalfred's take
|
/cc @ajayalfred to review design decisions here, although it should probably not block the PR. A similar decision was taken in #465, so it's probably a good idea to review both. |
|
Will be reviewing both today |
|
[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 |
* Add 'new run' button to Pipeline details page Needs test. * Adds tests for the NewRun page. Still needs tests for the PipelineDetails page * Adds tests for the PipelineDetails page * Use fromRunId as query param for embedded pipelines instead of pipelineFromRun * Refactors some NewRun tests and separates embedded pipeline handling in NewRun
Change-Id: I8d583a60e3fc6f1714a75f8040164d4ed7a0d829 Signed-off-by: Henry Wang <henry.wang@arm.com> Jira: ENTOS-1325
* Add disable-cache flag and default caching for compiler * Add updated testdata and features doc * Update docs * Add task level cache enabling * Remove disable-cache flag and add warning * Update docs * fix typo * Overwrite pipeline level label with task level label and add examples * Update imports * update format * Update features docs
Users will now be able to create a run directly from the pipeline details page.
If the pipeline was a regularly uploaded pipeline, the new run page will show the normal pipeline picker with the relevant pipeline preselected.
If the pipeline was part of a run created in a notebook or from the CLI, the new run page will instead show radio buttons allowing the user to continue with the relevant pipeline, or select a different one. Users can also change their minds about this and use the radio buttons to reselect the original pipeline.
Fixes #53
This change is