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

Transition start execution running #1056

Merged
merged 11 commits into from Dec 19, 2022
Merged

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Dec 15, 2022

Add transition since the execution is started until it is received from wanda. It uses a "fake it until make it" pattern which fakes the redux state setting an execution in a running state.
Now on, all wanda related views are under clusters_new/${clusterID} endpoint. This is done with the usingWanda boolean variable

demo

PD: I have fixed many small (and not so small) details on the way
PD2: The gif is a bit faked, as in order to work we need to merge #1032 first

@arbulu89 arbulu89 marked this pull request as ready for review December 15, 2022 16:29
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

Just a tiny comment on a test then we can merge

Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

Sorry, a couple things more 😅

@@ -158,6 +158,7 @@ function ChecksSelectionNew({ clusterId, cluster }) {
{localSavingSuccess && selectedChecks.length > 0 && (
<SuggestTriggeringChecksExecutionAfterSettingsUpdated
clusterId={clusterId}
usingWanda
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid referencing the raw name of the service? something like usingNewChecksEngine would be fine.

Also, we use the same pattern in the router. Having a shared name for all these props would allow us for a simpler grep in the future when we have to remove all of these switches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced all the places where we have this pattern by this usingWanda, so there is not any other wording.
This variable will be removed soonish, and I preferred the wanda wording, as it makes it cristal clear what it does.
But well, I will check if you prefer the other way around

Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +9 to +12
export const executionRequested = (clusterID, hosts, checks) => ({
type: EXECUTION_REQUESTED,
payload: { clusterID, hosts, checks },
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just requestExecution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's avoid this discussion now. We already have a tech debt ticket to talk about present/past term in the actions/events, etc

@arbulu89 arbulu89 force-pushed the transition-start-execution-running branch from 6406053 to 57a90f0 Compare December 19, 2022 09:20
Copy link
Member

@EMaksy EMaksy left a comment

Choose a reason for hiding this comment

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

Hey nice!
Looks good to me :D
Just a small note about the cluster test in a comment.

assets/js/state/selectors/cluster.test.js Show resolved Hide resolved
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

LGTM

@arbulu89 arbulu89 merged commit ae6f0b8 into main Dec 19, 2022
@arbulu89 arbulu89 deleted the transition-start-execution-running branch December 19, 2022 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

6 participants