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

Make runtype switch from "normal" to "async" a sticky setting #536

Merged
merged 9 commits into from
Aug 16, 2023

Conversation

spankywetfish
Copy link
Contributor

@spankywetfish spankywetfish commented Aug 11, 2023

In general use with multiple hosts it is (IMHO) more practical to have the jobs run asynchronously so that there is no need to wait for completion before performing the next task.

The changes made to RunType.js reflect that priority and default the job input to "async".
I've been running with this configuration for approximately 18 months with no known issues.

N.B.
Apologies if this is not the correct format for a pull request, it's a first for me.

[Erwin added:]
better solution is to remember the choice so that both styles are equally supported, see below for discussion.

@erwindon erwindon self-assigned this Aug 11, 2023
@erwindon
Copy link
Owner

erwindon commented Aug 11, 2023

@spankywetfish
Technically, the PR is fine.
But I cannot change the default behavior of a feature, unless it is an improvement for everyone.
I cannot accept this as is.
But all is not lost...
There are alternatives that should work for all:

  1. make the initial value based on a variable in the /etc/salt/master file
    rather technical and it would apply to all users
  2. make the initial value based on a variable in some *.txt file
    rather technical and it would apply to all users
  3. use 2 "Run command" buttons, one "Run command normal" and also "Run command async"
    no configuration, is per-use, but complicates GUI and conflicts with possible extensions in the future (e.g. batched runs).
  4. make the initial value based on a plain local-storage-item, set according to the latest use
    no configuration, is per-user/per-browser, still looks simple, must make the default more visible to prevent confusion. risk on information leak through use of storage-items is negligible.

how about number 4?

don't worry on who should build it. I can do that.

@erwindon erwindon marked this pull request as draft August 11, 2023 12:04
@erwindon erwindon assigned spankywetfish and unassigned erwindon Aug 11, 2023
@erwindon
Copy link
Owner

Additionally, I've created an implementation of proposal 4. See branch runtype4.

@spankywetfish spankywetfish marked this pull request as ready for review August 11, 2023 22:16
@spankywetfish
Copy link
Contributor Author

It looks like we've both been doing the same thing slightly different ways, I've also updated Api,js so that the runtype is persistent across sessions. I'd hazzard that as I consider myself in no way a programmer, yours implementation is more elegant.

@erwindon
Copy link
Owner

I've hidden my branch runtype4.
we're nearly there, please look at the comments with the js code...

@erwindon erwindon changed the title Switch default runtype from "normal" to "async" Make runtype switch from "normal" to "async" a sticky setting Aug 12, 2023
}
// Store last used runType
Utils.setStorageItem("local", "runtype", runType);
return runType;
Copy link
Owner

Choose a reason for hiding this comment

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

when a page-refresh is done, all GUI values are unavoidably lost.
this function would then reset the value to "normal" and also save it.

  • saving the value should only be done in function _updateRunTypeText, as that is the callback function from the dropdownlist.
  • instead of runType = "normal", the saved value should be retrieved: runType = Utils.getStorageItem("local", "runtype", "normal"); (which itself will have a default of 'normal')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to _updateRunTypeText, and suggested modification for retrieval applied, all tested and linted.

@sonarcloud
Copy link

sonarcloud bot commented Aug 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@erwindon erwindon merged commit 33e2c9e into erwindon:master Aug 16, 2023
5 checks passed
erwindon added a commit that referenced this pull request Aug 16, 2023
Make runtype switch from "normal" to "async" a sticky setting (left out remote history)
@erwindon
Copy link
Owner

for a next time...
please use a dedicated branch, which you can re-base until it is merged.
I rolled-back this merge and made it a regular commit.

@erwindon
Copy link
Owner

thanks for the addition!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants