remove hardcoded rootPath values

[Jeansen]

What is the problem I am trying to address?

Systemd script had some hard-coded values overwriting the config.toml settings. I've removed them and added some dynamic replacements in the athens.service

How is the fix applied?

?

What GitHub issue(s) does this PR fix or close?

Fixes #1798

[Jeansen]

I believe the install shell script can be much more robust in some places and some variables could be extracted to the top. Currently, the scrip also expects some files in the right place, instead of being relative to its location. I'll do that cleanup work in another PR. This PR's sole purpose is to remove the hard-coded values. One step at a time ;-)

[Ullaakut]

Shouldn't the chown and chgrp commands be kept in order to still make sure www-data owns the ReadWritePaths?

[Ullaakut]

Hi @Jeansen ! Are you still interested in getting this merged?

[Jeansen]

Hi @Ullaakut. Yes, I am. But I am not sure, how to proceed...

[matt0x6F]

Hi @Ullaakut. Yes, I am. But I am not sure, how to proceed...

Heya! Do you mind ELI5'ing these changes as well as your long term plan? I'm actually pretty unsure of how all of this should work. It might also be helpful to have a main issue for the long term plan as well as some sub-issues since you want to tackle this in phases.

[Jeansen]

Heya! Do you mind ELI5'ing these changes as well as your long term plan? I'm actually pretty unsure of how all of this should work. It might also be helpful to have a main issue for the long term plan as well as some sub-issues since you want to tackle this in phases.

I'll try! This PR removes hard-coded values. The only place of truth should the the config TOML file. If this is in agreement with you (all), then that's what this PR is about.

To make things more robust, I see some more possible improvements in the install script. But since this is my first PR in the project, I first wanted to get some idea of how this project "ticks". Anyway, if you think my ideas are worth a shot, I could create another PR so you see, where I'd like to go.

But as it stands, it is all about having as much hard-coded values removed or extracted to only one place and making the installation follow Postel's Law.

[matt0x6F]

That makes sense! Thanks for the breakdown.

Does it make sense to ensure whatever directory the user has supplied that it fits the permissions we need and that it actually exists? I'm referring to this being removed:

[Jeansen]

I think users should be aware of the fact that they are missing the right permissions and be forced to set them explicitly. Convention over configuration is a great concept. But with respect to permissions I'd be careful ;-)

[matt0x6F]

Thanks for that bit of wisdom.

Last thing, do you think anything here needs changing?

[Jeansen]

@matt0x6F I'll have a look at it this WE and give you my feedback then.

[Jeansen]

I checked the helm repo. It looks fine to me, so far. But I did not dive into details too far, to be honest.

Anyway, there you have it, too. The user ist set to 1000 if not run as root. And the emptyDir volume ist mountet with the same user/group since there is not explicit fsGroup set. So, no access problems here.

Also, helm templates kind of serve as a on-the-fly config file by creating all the relevant environment variables which - if applicable - overwrite settings from the config file. Regarding this I'd strongly advice against being able to set a user/group through environment variables. But I would put the athens user and group as a default value in config.tom. And like I wrote already, if a non-cluster setup fails, then a user can either change user and group settings or create the athens user and group.

I also saw the issue requesting an option to add a custom config.toml file. I'd also recommend against it. If anyone needs a custom config file in place, one can always create a custom Docker image base on the athens image. Otherwise I think everything I see so far is aligned with a 12-factor app design which also means that all relevant settings can be set through environment variables.

[matt0x6F]

That makes sense. I've got a couple PRs to merge later today so this one's on my list!

[Jeansen]

I believe the install shell script can be much more robust in some places and some variables could be extracted to the top. Currently, the scrip also expects some files in the right place, instead of being relative to its location. I'll do that cleanup work in another PR. This PR's sole purpose is to remove the hard-coded values. One step at a time ;-)

I'll go for the next phase then, do some more improvements, cleanup etc - if that's OK?