Fix missing run and pipeline id when buttons are clicked before content load #2584
Conversation
|
Hi @drewbutlerbb4. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@Bobgy I have added the tests you requested and some additional ones that seemed to be missing. This is my first interaction with Jest so let me know of any comments you have. |
|
@drewbutlerbb4 thanks! I haven't looked into details, can you fix lint errors in https://github.com/kubeflow/pipelines/pull/2584/checks?check_run_id=295153268? You can also test lint errors locally by running |
There was a problem hiding this comment.
This is great! Thanks for also filling in tests for those missing parts. I really appreciate that.
You've been doing great with jest. The tests have no problems and align their style with existing tests too. Good job!
Regarding contribution experience, did you find anything confusing or feel lost at some point? I'm working on #2407 which aims to improve contribution experience. Your feedback will be helpful.
| @@ -416,7 +416,7 @@ describe('PipelineDetails', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| it('clicking new run button when viewing half-loaded page navigates to the new run page with run ID', async () => { | |||
| it('clicking new run button when viewing half-loaded page navigates to the new run page with pipeline ID', async () => { | |||
|
/ok-to-test |
|
Lint error is fixed.
Nope, the documentation is pretty clear and the mock-api is super useful. No complaints from me. |
|
Cool, note there's one npm test failing. |
|
@drewbutlerbb4 Thanks! /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bobgy 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 |
* Upgrade the K8s version to 1.24 for tests Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com> * Switch steps to build a minikube cluster from manual installation to using manusa/actions-setup-minikube Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com> * Install packages for port-forward Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com> * Add GITHUB_TOKEN to minikube-setup Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com> * Manially download seldonio/mlserver Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com> * Use docker.io/seldonio/mlserver:1.0.1-slim for test Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com> * Upgrade the minikube action to 2.7.2 Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com> Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com>
Fixes an issue related to #2488.
If you click certain buttons before the full page has loaded then there will be adverse results due to no pipeline/run id being specified. An example is clicking the "Delete" button on the pipeline details page before the page has fully loaded will result in receiving a popup saying "Delete succeeded for this pipeline", but nothing will be deleted.
In the pipeline details page the "Create Experiment" and "Delete" buttons were changed to include the pipeline id.
In the run details page the "Retry", "Clone Run", and "Terminate" buttons were changed to include the run id.
My only concern is that I am not sure if the api is fail safe in regards to being asked to terminate a run for a run id which is already finished running. This is possible because the Terminate button is enabled on page load and now also has the run id on page load. If this is undesirable the Terminate button can be disabled until the page loads.
This also fixes a small typo in the mock-api. Reupload of #2575, my apologies for the mess.
This change is