Skip to content
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

fix(ui): Cost Opt should only apply to live Workflows #12170

Merged

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Nov 9, 2023

Fixes #12160

Motivation

  • after the unification of the Archived + Live Workflow APIs and UIs in feat: Unified workflows list UI and API #11121, the WorkflowsList's CostOptimisationNudge was not altered to account for the additional Archived Workflows
    • as such, it started showing up even for users with solid GC and archiving configurations

Modifications

  • change the counting logic to skip archived workflows, which are explicitly excluded in the docs

  • small code fixes/optimizations:
    • improve typings
    • handle non-existent labels or metadata in isArchivedWorkflow func same as in countsByCompleted func
      • and slightly refactor / clean-up the logic with optional chaining
  • small grammar fix: comma splice in the CostOptimisationNudge message itself, where there is already a conjunction

  • small docs clarifications and grammar fixes:
    • consistently use "the" Workflow Archive, not "a" Workflow Archive
      • there is only one and the Workflow Archive page uses "the"
    • Workflow Archive is not an "or" with deleting Workflows, it would be an "and"
      • but it is optional, so split that into a second sentence with "you can"
    • "enable [...] and you can still view them" was confusing wording as it used a conjunction (on top of the incorrect conjunction above) instead of a preposition
      • change to "enable [...] to continue viewing"

Verification

Tested locally by reducing the count to 10. It correctly skips the 1 archived workflow I have in the list. Screenshot:

Screenshot 2023-11-08 at 9 10 30 PM

- after the unification of the Archived + Live Workflow APIs and UIs in 82310dd, the `WorkflowsList`'s `CostOptimisationNudge` was not altered to account for the additional Archived Workflows
  - as such, it started showing up even for users with solid GC and archiving configurations
  - change the counting logic to skip archived workflows, which are explicitly excluded in the docs
    - using the Workflow Archive is one of the recommended options in fact

- small grammar fix: comma splice in the `CostOptimisationNudge` message itself, where there is already a conjunction
- small docs clarifications and grammar fixes:
  - consistently use "the" Workflow Archive, not "a" Workflow Archive
    - there is only one and the Workflow Archive page uses "the"
  - Workflow Archive is not an "or" with deleting Workflows, it would be an "and"
    - but it is optional, so split that into a second sentence with "you can"
  - "enable [...] and you can still view them" was confusing wording as it used a conjunction (on top of the incorrect conjunction above) instead of a preposition
    - change to "enable [...] to continue viewing"

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
`isArchivedWorkflow` should handle it same as `countsByCompleted` already does

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 added the area/docs Incorrect, missing, or mistakes in docs label Nov 9, 2023
@terrytangyuan terrytangyuan merged commit 873bacb into argoproj:master Nov 9, 2023
16 checks passed
@agilgur5 agilgur5 deleted the fix-ui-workflow-list-cost-opt branch November 9, 2023 15:00
terrytangyuan pushed a commit that referenced this pull request Nov 27, 2023
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 added this to the v3.5.x patches milestone Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Incorrect, missing, or mistakes in docs area/ui area/workflow-archive
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI: 3.5 list view warns about 500 succeeded archived workflows
2 participants