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

Updated Install/Uninstall scripts to resolve #120 #123

Merged
merged 3 commits into from
Oct 22, 2017

Conversation

SirLagz
Copy link
Collaborator

@SirLagz SirLagz commented Oct 21, 2017

Issue #120 is regarding our install scripts overwriting the user's rc.local file.
I've updated the install and uninstall scripts now to rectify this issue.
Installing will now append to the rc.local file, rather than moving our own one over the user's one.
A backup is also made before which wasn't happening previously.

The uninstall script has the option to either restore a backup or remove the lines added by our install script.
Also fixed a missing 'read answer' statement, and fixed up some styling for the Web console.

@SirLagz SirLagz requested review from billz and jrmhaig October 21, 2017 14:47
Copy link
Collaborator

@jrmhaig jrmhaig left a comment

Choose a reason for hiding this comment

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

After I saw a lot of recent activity on this project I had a look and saw you requested my review on this PR! (Either by accident or intentionally)
Sorry I've been a bit quiet but I have been busy with other things. I'll keep checking back from time to time and see if I can contribute again at some point.
Great to see things moving!

@@ -155,7 +160,24 @@ function default_configuration() {
sudo mv $webroot_dir/config/hostapd.conf /etc/hostapd/hostapd.conf || install_error "Unable to move hostapd configuration file"
sudo mv $webroot_dir/config/dnsmasq.conf /etc/dnsmasq.conf || install_error "Unable to move dnsmasq configuration file"
sudo mv $webroot_dir/config/dhcpcd.conf /etc/dhcpcd.conf || install_error "Unable to move dhcpcd configuration file"
sudo mv $webroot_dir/config/rc.local /etc/rc.local || install_error "Unable to move rc.local file"
#sudo mv $webroot_dir/config/rc.local /etc/rc.local || install_error "Unable to move rc.local file"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the file config/rc.local be deleted from the repository?
Also, delete this line, rather than leave it commented out, if it is not needed any more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I requested it intentionally :) I haven't had much involvement in this apart from creating it initially until now so wanted your input on this :)

Yes, we can delete the config file. I was meant to delete that line as well before I committed it. Oops haha.

@jrmhaig
Copy link
Collaborator

jrmhaig commented Oct 21, 2017

Also, while you are changing the install script you might check the packages that are installed by apt. I'm not entirely sure but I think php5-cli needs to be changed to php7.0-cli. If there is a non-version specific package that gets the latest php then that might be better.

@SirLagz
Copy link
Collaborator Author

SirLagz commented Oct 21, 2017

Yes, there seems to be a non-version-specific package for Stretch. Not sure about Jessie though.

@jrmhaig
Copy link
Collaborator

jrmhaig commented Oct 22, 2017

Regarding the version of php-cgi (not php-cli as I wrote above), I have created a separate PR for discussion - #124

@SirLagz
Copy link
Collaborator Author

SirLagz commented Oct 22, 2017

I'll remove the rc.local configuration file and remove the commented out lines in the install script then, and merge the changes.

@SirLagz SirLagz merged commit 6d90293 into RaspAP:master Oct 22, 2017
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