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

Add manual_config install var to cli #1208

Merged
merged 2 commits into from
Dec 16, 2023
Merged

Add manual_config install var to cli #1208

merged 2 commits into from
Dec 16, 2023

Conversation

Gamerboy59
Copy link
Contributor

Make the manual_config var, which is available to the web installer, usuable for the cli installer too. If manual_config is set to true skip else (not set or false) proceed with auto config.

Maybe example in docs needs to be updated, too.

Make the manual_config var, which is available to the web installer, usuable for the cli installer too. If manual_config is set to true skip else (not set or false) proceed with auto config.
@d00p d00p self-assigned this Dec 16, 2023
@d00p
Copy link
Member

d00p commented Dec 16, 2023

Correction: the cliinstaller already creates the userdata.inc.php on step 3 - so i guess it's fine to skip the configuration

@Gamerboy59
Copy link
Contributor Author

Correction: the cliinstaller already creates the userdata.inc.php on step 3 - so i guess it's fine to skip the configuration

I'm not quite sure if I understand your correction statement correctly. Is it fine to skip the config like in this PR or do you want it differently or is it already skipped?

@d00p
Copy link
Member

d00p commented Dec 16, 2023

I was kind of thinking out loud sorry if it was a bit confusing.

No, basically it's all fine, I would do it a bit differently, don't really like a ternary operator within an if-statement. Something like this:

-                               if (!empty($decoded_input) || $this->io->confirm('Execute command now?', false)) {
-                                       passthru($cmdfield['value']);
+                               if (!empty($decoded_input) &&
+                                       (!isset($decoded_input['manual_config']) || (bool)$decoded_input['manual_config'] === false)
+                               ) {
+                                       if ($this->io->confirm('Execute command now?', false)) {
+                                               passthru($cmdfield['value']);
+                                       }
                                }

what do you think?

@Gamerboy59
Copy link
Contributor Author

Gamerboy59 commented Dec 16, 2023

We can do it without ternary but in your example the confirm dialog would always appear which is not good for fully unattended runs. The passthru cmd should be executed even if the manual_config variable is missing or false and don't cause a prompt. The prompt should only appear if the $decoded_input is emtpy.

@d00p
Copy link
Member

d00p commented Dec 16, 2023

Yes, you're right, this should do:

if (!isset($decoded_input['manual_config']) || (bool)$decoded_input['manual_config'] === false) {
	if (!empty($decoded_input) || $this->io->confirm('Execute command now?', false)) {
		passthru($cmdfield['value']);
	}
}
  1. check for "manual_config" to be unset or at least "false", otherwise dont run anything
  2. If an input file was given OR the confirmation was answered, run command

@Gamerboy59
Copy link
Contributor Author

Gamerboy59 commented Dec 16, 2023

Yes thanks, this looks like a disassemblement of the ternary version.
I've updated the PR.

@d00p d00p merged commit 55a2ae3 into froxlor:main Dec 16, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants