[WIP] Add new cancel states for "graceful" cancel #2378

williamlfish · 2022-05-24T01:29:21Z

Changes

Updates to use the new cancel statuses, with backwards compatibility for pipeline controllers that have not updated to consume them. There are still some changes needed for the wording around the new statuses from a ux perspective. A lot has been discussed in the issue here.

One of the ideas was to add an info icon with a tool tip for and explanation, and from what I can tell the carbon radio buttons break when trying to add the icons ( I don't do a lot of front end, so could be a me problem 😅 ). So there is still the need to explain them, and a way to present. So I admit, this pr may not be ready, but perhaps a conversation starter?

Juggling the version, and the status passed in is a little cumbersome and hopefully something that can be removed relatively soon.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)

tekton-robot · 2022-05-24T01:29:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign alangreene
You can assign the PR to them by writing /assign @alangreene in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot · 2022-05-24T01:29:36Z

Hi @williamlfish. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

williamlfish · 2022-05-24T01:53:00Z

@AlanGreene The pr we discussed a while back. Sorry it took me a bit to get to it 😅 but hopefully we can start from here? At the end of the day, just wanted to have it on your radar. thanks!

AlanGreene · 2022-05-25T10:26:08Z

Thanks for the PR @williamlfish. I've just taken a very quick look and it seems like a solid start. It'll likely be a few days before I can do a proper review but I'll try to give some early feedback soon.

/ok-to-test

tekton-robot · 2022-05-25T10:31:18Z

@williamlfish: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-tekton-dashboard-unit-tests	`3013636`	link	`/test tekton-dashboard-unit-tests`
pull-tekton-dashboard-integration-tests	`3013636`	link	`/test pull-tekton-dashboard-integration-tests`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

tekton-robot · 2022-05-25T10:31:18Z

@williamlfish: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-tekton-dashboard-unit-tests	`3013636`	link	`/test tekton-dashboard-unit-tests`
pull-tekton-dashboard-integration-tests	`3013636`	link	`/test pull-tekton-dashboard-integration-tests`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

AlanGreene

I'm seeing some odd behaviour where the state sent in the API call doesn't always match the selected option in the UI. Possibly in part related to mismatch between the value in the local component state and the value in localStorage, commented below with a suggestion to address that. There may be some other issues causing the mismatch too.

That said the overall structure looks good. Just some styling / content issues to take care of as previously discussed but we're good to proceed with the approach 👍

Thanks for tackling this @williamlfish and apologies for the delay getting around to reviewing 😸

AlanGreene · 2022-05-31T12:32:43Z

packages/components/src/components/RadioGroup/RadioGroup.js

@@ -0,0 +1,65 @@
+/*


I'm not sure we need this new component since it's only used in one place and just wraps existing components with no additional logic.

AlanGreene · 2022-05-31T12:34:59Z

src/api/pipelineRuns.test.js

+  return API.cancelPipelineRun({
+    name,
+    namespace,
+    cancelState: payload[0].value


Let's extract this to a variable, const cancelState = 'PipelineRunCancelled'; and use it in both places. It makes it easier to read without having to backtrack to parse the payload object.

AlanGreene · 2022-05-31T12:36:48Z

src/containers/PipelineRuns/PipelineRuns.js

+      defaultMessage: 'Cancel'
+    });
+
+    const canceledWithFinally = intl.formatMessage({


cancelledWithFinally - double 'l' for consistency with the state value. Same in a few other places too

AlanGreene · 2022-05-31T12:38:00Z

src/containers/PipelineRuns/PipelineRuns.js

+          secondaryButtonText: intl.formatMessage({
+            id: 'dashboard.modal.cancelButton',
+            defaultMessage: 'Cancel'
+          }),


no need for this, it's the default text on the secondary button already

AlanGreene · 2022-05-31T12:41:32Z

src/containers/PipelineRuns/PipelineRuns.js

+    setCancelState(state);
+    setDefaultCancelState(state);


ideally we would only be setting this in one place and reading from that single place consistently.

We could use a similar pattern to the one in ListPageLayout for the pagination settings. Set the value in component state, useEffect to listen for changes and write to localStorage. This way they should always be in sync and there's one trigger for the update.

We would read from localStorage on mount and use it to populate the initial value in useState. From that point on the component state is our source of truth, until the page is reloaded.

AlanGreene · 2022-05-31T12:43:42Z

src/utils/index.js

@@ -214,3 +214,31 @@ export function setTheme(selectedTheme = getTheme()) {
  );
  localStorage.setItem('tkn-theme', theme);
 }
+
+function sanatizeCancelState(state) {


typo: sanitize

AlanGreene · 2022-05-31T12:45:01Z

src/utils/index.js

+  if (valid) {
+    return state;
+  }
+  return 'PipelineRunCancelled';


We have a different default here vs. the end of getDefaultCancelState below which calls this function. Should they be consistent / combined?

williamlfish · 2022-06-01T01:16:55Z

@AlanGreene thanks for the review, and no worries about the delay, we are all busy 😅! Speaking of busy, I should have time to get to these changes before next week. thanks again!

williamlfish · 2022-06-06T14:45:29Z

@AlanGreene sorry for not having this out! My household got a case of the vid, so the rest of us have been hunkering down and testing everyday. All that said, I don't have childcare at the moment and free time is out the window. I'll get to it as soon as i can, but I don't want to be a blocker either so if you or anyone else is interested, please feel free to add to this. Thanks!

AlanGreene · 2022-06-06T15:21:10Z

No worries at all, look after yourself and your family.

AlanGreene · 2022-06-08T14:19:14Z

src/containers/PipelineRuns/PipelineRuns.js

+  const versionCheck = parseInt(pipelinesVersion.split('.')[1], 10);
+  const allowCancelOptions = !Number.isNaN(versionCheck) && versionCheck > 34;


note to self: another option would be to use something like semver to perform the comparison, especially if we're going to end up with similar checks for other features. This could help with compatibility checks especially as Tekton Pipelines starts removing some of the v1alpha1 APIs (e.g. Conditions)

e.g.

const minVersion = '>=0.35.0'; const allowCancelOptions = semver.satisfies(pipelinesVersion, minVersion);

Depending on the size the package contributes to the build output we may want to import only the specific function used, there are some docs included for how to achieve that.

tekton-robot · 2022-07-06T00:11:08Z

@williamlfish: PR needs rebase.

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.

William Fish and others added 4 commits May 16, 2022 15:37

add the radios

fac180a

missing files

2e0eba2

add functionality to the radios

7387f60

linting

eb2e4fa

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 24, 2022

tekton-robot requested review from briangleeson and LyndseyBu May 24, 2022 01:29

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2022

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 24, 2022

williamlfish changed the title ~~Add new cancel states for~~ Add new cancel states for "graceful" cancel May 24, 2022

resolve conflicts

2c0bb04

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2022

williamlfish added 5 commits May 23, 2022 21:40

unlink fix

cb2fb6a

fix lock and remove not used icon

e218693

remove lock that was never there

7d46681

clean up comment

e8de91a

clean up comment

3013636

williamlfish changed the title ~~Add new cancel states for "graceful" cancel~~ [WIP] Add new cancel states for "graceful" cancel May 24, 2022

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 24, 2022

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 25, 2022

AlanGreene requested changes May 31, 2022

View reviewed changes

AlanGreene reviewed Jun 8, 2022

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2022

AlanGreene mentioned this pull request Jul 6, 2022

Add graceful cancel options for PipelineRuns #2398

Merged

4 tasks

tekton-robot closed this in #2398 Jul 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add new cancel states for "graceful" cancel #2378

[WIP] Add new cancel states for "graceful" cancel #2378

williamlfish commented May 24, 2022 •

edited

Loading

tekton-robot commented May 24, 2022

tekton-robot commented May 24, 2022

williamlfish commented May 24, 2022

AlanGreene commented May 25, 2022

tekton-robot commented May 25, 2022

tekton-robot commented May 25, 2022

AlanGreene left a comment

AlanGreene May 31, 2022

AlanGreene May 31, 2022

AlanGreene May 31, 2022

AlanGreene May 31, 2022

AlanGreene May 31, 2022 •

edited

Loading

AlanGreene May 31, 2022

AlanGreene May 31, 2022

williamlfish commented Jun 1, 2022

williamlfish commented Jun 6, 2022

AlanGreene commented Jun 6, 2022

AlanGreene Jun 8, 2022

tekton-robot commented Jul 6, 2022

		const versionCheck = parseInt(pipelinesVersion.split('.')[1], 10);
		const allowCancelOptions = !Number.isNaN(versionCheck) && versionCheck > 34;

[WIP] Add new cancel states for "graceful" cancel #2378

[WIP] Add new cancel states for "graceful" cancel #2378

Conversation

williamlfish commented May 24, 2022 • edited Loading

Changes

Submitter Checklist

tekton-robot commented May 24, 2022

tekton-robot commented May 24, 2022

williamlfish commented May 24, 2022

AlanGreene commented May 25, 2022

tekton-robot commented May 25, 2022

tekton-robot commented May 25, 2022

AlanGreene left a comment

Choose a reason for hiding this comment

AlanGreene May 31, 2022

Choose a reason for hiding this comment

AlanGreene May 31, 2022

Choose a reason for hiding this comment

AlanGreene May 31, 2022

Choose a reason for hiding this comment

AlanGreene May 31, 2022

Choose a reason for hiding this comment

AlanGreene May 31, 2022 • edited Loading

Choose a reason for hiding this comment

AlanGreene May 31, 2022

Choose a reason for hiding this comment

AlanGreene May 31, 2022

Choose a reason for hiding this comment

williamlfish commented Jun 1, 2022

williamlfish commented Jun 6, 2022

AlanGreene commented Jun 6, 2022

AlanGreene Jun 8, 2022

Choose a reason for hiding this comment

tekton-robot commented Jul 6, 2022

williamlfish commented May 24, 2022 •

edited

Loading

AlanGreene May 31, 2022 •

edited

Loading