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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

amoskong
Copy link
Contributor

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 [email protected]

install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
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.

 # Without this fix:
EnvironmentFile=/home/scylla-test/scylladb/etc/sysconfig//scylla-jmx

 # Applied this fix:
EnvironmentFile=/home/scylla-test/scylladb/etc/sysconfig/scylla-jmx

v2: don't translate the path in executing install.sh, it has problem
in building rpm/deb if /etc/sysconfig is a symlink. (Avi)

Signed-off-by: Amos Kong <[email protected]>
@syuu1228
Copy link
Contributor

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:/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
Copy link
Contributor Author

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:/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
Copy link
Contributor

penberg commented Jan 4, 2021

@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
Copy link
Contributor Author

amoskong commented Jan 4, 2021

@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: scylladb/scylladb#7860

@avikivity
Copy link
Member

@syuu1228 are you happy with this?

@penberg
Copy link
Contributor

penberg commented Mar 4, 2021

@syuu1228 ping

Copy link
Contributor

@syuu1228 syuu1228 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants