oupala / apaxy

a simple, customisable theme for your apache directory listing
https://oupala.github.io/apaxy/
GNU General Public License v3.0
1.86k stars 256 forks source link

feat: add install script with instructions #124

Closed ghost closed 5 years ago

ghost commented 5 years ago

fixes #107

proposed changes

oupala commented 5 years ago

In a first standard test, everything works like a charm. Good point!

I have to test it more deeply, but it looks good!

oupala commented 5 years ago

Here is what I had to change in the config file:

Here are the changes after executing the script:

oupala commented 5 years ago

I did not test the "Copying files to web root..." feature as I don't want an unknown script to mess with my system. I think that this feature should be optional and disabled by default. It should be enabled using something like an --install flag.

oupala commented 5 years ago

It seems that the script is not idempotent and cannot be run twice without outputting an error. I think that line 27 return code should be checked.

cp -r . "$WEB_ROOT$INSTALL_DIRECTORY"

And maybe some other commands.

Additionnaly, it appears that the script tries to copy the .git directory, if apaxy has been cloned by git. The line 27 is - here again - copying to much things to the destination directory.

oupala commented 5 years ago

As far as I tested, the install feature works.

But I think this is a problem to link the configure feature with the install feature.

There is a bunch of usecases where you don't want to install apaxy, you juste want to configure it:

Moreover, the files used are named apaxy.config and apaxy-configure.sh. Hence their job is to configure apaxy, not to install it.

So, in my opinion we should split the two features. Either we have 2 scripts (apaxy-configure.sh and apaxy-install.sh) and one script can launch the other one (probably the install script should call the configure script). Either we keep the current script, but we add an option (which is false by default) to tell if we want to install apaxy or not after configuring it (--install true or --install false, where false is the default value whenever the parameter is not set on the command line).

What do you think of that @jordanbancino?

It could be good to close this merge request in a near futur so that we can fix the Dockerfile issue (#115) by using this configure script in the Docker image build.

ghost commented 5 years ago

Yep, I completely agree with everything you’ve said here. I am so sorry, but I just simply don’t have the time to help out with this project. Maybe over the summer I can take a look again and fix it up. Just not now, or any time soon, I have a lot going on. I do appreciate being mentioned in these, so I can keep up with the project though. I definitely think the changes you’re proposing are important and should be looked at, though.

oupala commented 5 years ago

If I understand well, I can try to fix it myself. But if I don't, you might do that in a few months.

Am I right?

ghost commented 5 years ago

Yes, that sounds good. I would love to help out, and maybe I can do something every once and a while, there’s just a lot going on for me that I need to figure out first. Summer is the best time for me to get stuff done, though with work and college and stuff, it may be hard.

oupala commented 5 years ago

Could you please add me to your apaxy repo so that I can add content to your pull request?

Otherwise I can start a new pull request or branch, but all the useful comments from this pull request will miss us.

ghost commented 5 years ago

Yep, no problem, I just did. 👍 My schedule should be clearing up just a tad here soon so I may be able to help, but no guarantees yet.

oupala commented 5 years ago

I have largely rewrote the install process and I have tester many usecases:

Could you please tell me if you have time to test it a bit before I merge the pull request?

oupala commented 5 years ago

It seems that you don't time to test this pull request at this moment. I'll merge this pull request as I have other works that depend on this install script.

Feel free to test it later and to open issues if you find any problem.