search

scylladb / scylla-jmx

Scylla JMX proxy
GNU Affero General Public License v3.0
28 stars 52 forks source link

install.sh: fix hardcode sysconfdir in EnvironmentFile path #150

Open amoskong opened 3 years ago

amoskong commented 3 years ago

Currently we use fixed sysconfdir name 'sysconfig', it works as expected. But let's change it to use the assigned sysconfdir, it still works when a different sysconfdir is assigned.

Signed-off-by: Amos Kong amos@scylladb.com

syuu1228 commented 3 years ago

Oh sorry it seems like why we requires this change is because I haven't synced changes from scylla repo to scylla-jmx repo. Anyway, I think we align the way to implement this between scylla repo and scylla-jmx repo. On scylla repo, we do:

        cat << EOS > "$rsystemd"/scylla-server.service.d/nonroot.conf
[Service]
EnvironmentFile=
EnvironmentFile=$rsysconfdir/scylla-server

https://github.com/scylladb/scylla/blob/master/install.sh#L372 So scylla-jmx repo should be same. If current scylla repo's implementation has problem, then replace with new one on both repos, not just one.

amoskong commented 3 years ago

Oh sorry it seems like why we requires this change is because I haven't synced changes from scylla repo to scylla-jmx repo. Anyway, I think we align the way to implement this between scylla repo and scylla-jmx repo. On scylla repo, we do:

        cat << EOS > "$rsystemd"/scylla-server.service.d/nonroot.conf
[Service]
EnvironmentFile=
EnvironmentFile=$rsysconfdir/scylla-server

https://github.com/scylladb/scylla/blob/master/install.sh#L372 So scylla-jmx repo should be same. If current scylla repo's implementation has problem, then replace with new one on both repos, not just one.

Currently implement in scylla_repo is fine, a redundant backslash is harmless.

# Current scylla_repo::
EnvironmentFile=/home/scylla-test/scylladb/etc/sysconfig//scylla-server

# Fixed scylla-jmx:
EnvironmentFile=/home/scylla-test/scylladb/etc/sysconfig/scylla-jmx

So it's fine to only merge this PR.

penberg commented 3 years ago

@amoskong Even if scylla.git works fine, let's make sure the two solutions are the same. So please open a pull request in scylla.git to switch to also use realpath and let's merge both.

amoskong commented 3 years ago

@amoskong Even if scylla.git works fine, let's make sure the two solutions are the same. So please open a pull request in scylla.git to switch to also use realpath and let's merge both.

Thanks for the suggestion, I had sent a PR: https://github.com/scylladb/scylla/pull/7860

avikivity commented 3 years ago

@syuu1228 are you happy with this?

penberg commented 3 years ago

@syuu1228 ping