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

Replace "run now" CustomAction with standard action PerformSingleExecution #7165

Merged
merged 11 commits into from
Dec 4, 2024

Conversation

mgoworko
Copy link
Contributor

@mgoworko mgoworko commented Nov 18, 2024

Describe your changes

The main purpose of this PR, is to refactor "run now" action for periodic DeploymentManagers:

  • before: it was a CustomAction
  • now: it is a standard actions, similar to CANCEL, DEPLOY, etc.

Planned changes related to the PeriodicDeploymentManager:

  1. This change modifies basic handling and UX of "run now" action
    1A. Completely remove CustomAction mechanism, as it will be unused after stage 1 "run now"
  2. Second phase (already in progress while writing this comment) will involve:
  • moving the periodic deployment manager closer to Nu core
    • database, that was separate, moved the main Nu db
    • deployment manager decoupled from Flink
    • interface moved to the deployment manager API module, but mostly as-is, without huge changes
  • improving and optimizing queries about periodic scenarios and their status
  1. Third phase may involve further integrating the periodics into the general deployment manager interface - phase 2 is preparation for that.

Most important changes:

  1. The "run now" actions is now implemented as standard action:
  • it has its own dedicated button on actions panel, no longer uses generic CustomAction mechanism and button
  • this new button is now fully controlled by Nu backend response, both
    • its visibility (only for periodic processes)
    • active/inactive (greyed out), it is active only when newest version of scenario is scheduled
  1. The ProcessStateDefinitionManager improvements:
  • it is now aware, which version of the scenario is newest, and which is deployed
  • it allows to manage allowed actions, depending on deployed and latest version (it was needed for "run now" action)
  • ProcessStateDefinitionManager now allows to define, which action buttons are visible on FE (def applicableActions) (right now only applied to "run now" for periodic processes)
  • ProcessStateDefinitionManager now allows to specify custom tooltips for action buttons (again, it was needed by "run now" action")
  1. The PeriodicProcessStateDefinitionManager changes:
  • "run now" is now a standard action, handled in the graph of state transitions
  • "run now" is available only when newest version of scenario is scheduled
  • in other states, custom tooltips are returned, describing why it is now possible to invoke "run now"

Checklist before merge

  • Related issue ID is placed at the beginning of PR title in [brackets] (can be GH issue or Nu Jira issue)
  • Code is cleaned from temporary changes and commented out lines
  • Parts of the code that are not easy to understand are documented in the code
  • Changes are covered by automated tests
  • Showcase in dev-application.conf added to demonstrate the feature
  • Documentation added or updated
  • Added entry in Changelog.md describing the change from the perspective of a public distribution user
  • Added MigrationGuide.md entry in the appropriate subcategory if introducing a breaking change
  • Verify that PR will be squashed during merge

Summary by CodeRabbit

  • New Features

    • Introduced PerformSingleExecution action, now supported in the DeploymentManager and GUI.
    • Added new command type DMPerformSingleExecutionCommand for processing single execution requests.
    • Enhanced process state management with new fields for visible actions and action tooltips.
    • New PerformSingleExecutionButton added to the toolbar for executing single actions.
    • Improved tooltip management for actions based on current process state.
    • Updated API endpoints for better error handling and specificity in responses.
  • Bug Fixes

    • Improved error handling and logging for health check endpoints.
  • Documentation

    • Updated changelog and migration guide to reflect new features and changes in command structure.
    • Added new test cases for tooltip visibility based on process states.

This comment was marked as outdated.

@github-actions github-actions bot added client client main fe ui labels Nov 18, 2024
@mgoworko

This comment was marked as outdated.

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

@github-actions github-actions bot added the docs label Nov 19, 2024
@mgoworko mgoworko changed the title Improve "run now" state handling and treat it as a standard action, not custom action Replace "run now" CustomAction with standard action PerformSingleExecution, improve state handling and notifications Nov 19, 2024
@mgoworko mgoworko changed the title Replace "run now" CustomAction with standard action PerformSingleExecution, improve state handling and notifications Replace "run now" CustomAction with standard action PerformSingleExecution Nov 19, 2024
@mgoworko mgoworko marked this pull request as ready for review November 19, 2024 10:00
coderabbitai[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

@mgoworko mgoworko force-pushed the improve-run-now-state-handling branch from 879fd84 to d3c0e7d Compare November 20, 2024 15:23
coderabbitai[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

Copy link
Member

@arkadius arkadius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments added

disabled={name === "run now" ? disabledValue : false}
type={type}
/>
<CustomActionButton action={action} processName={processName} disabled={false} processStatus={status} type={type} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I see, there is no option to disable a custom action based on FE types. Do we need this disabled flag?

Copy link
Contributor Author

@mgoworko mgoworko Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is part of common ToolbarButtonProps, used for all buttons. AFAIK this value must be provided. I guess at the moment it is the only button, that is never disabled on FE side, so constant false provided here

After Arek's comments I added internationalization of tooltip messages, I think it needs FE review too. <-this change is reverted

coderabbitai[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

@mgoworko mgoworko requested review from arkadius and Dzuming November 28, 2024 08:51
This reverts commit aeaec47
…ate-handling

# Conflicts:
#	docs/Changelog.md
#	docs/MigrationGuide.md
coderabbitai[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

Copy link
Member

@arkadius arkadius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PeriodicProcessServiceIntegrationTest failed. Is it related with your change?

@mgoworko
Copy link
Contributor Author

mgoworko commented Dec 3, 2024

PeriodicProcessServiceIntegrationTest failed. Is it related with your change?

Seems completely not related. My change did not modify how periodic processes work internally. Logs are not conclusive, but I think it was Flink deployment error that caused it. It passes locally and passed on retry.

@arkadius
Copy link
Member

arkadius commented Dec 3, 2024

PeriodicProcessServiceIntegrationTest failed. Is it related with your change?

Seems completely not related. My change did not modify how periodic processes work internally. Logs are not conclusive, but I think it was Flink deployment error that caused it. It passes locally and passed on retry.

OK, so let's merge it

@mgoworko mgoworko merged commit 9db6f47 into staging Dec 4, 2024
18 checks passed
@mgoworko mgoworko deleted the improve-run-now-state-handling branch December 4, 2024 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client main fe docs ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants