FAForever / faf-stack

Aggregate project that allows to set up and manage a complete FAF environment within minutes
16 stars 18 forks source link

Update README.md #177

Closed BlackYps closed 3 years ago

BlackYps commented 3 years ago

Correct wrong bash command

Sheikah45 commented 3 years ago

What system were you running on that the -R didn't traverse the directory?

p4block commented 3 years ago

There's a lot of ways to achieve what that command implies (renaming the config.example folder but keeping the old one) on modern cp. Another tool such as rsync or an older cp uses more complex logic when working with directories, but even then it would still do the same thing (afaik)

What does this PR hope to achieve?

BlackYps commented 3 years ago

The problem with the current command is that it copies the config.template (and its contents) into the config folder. So you have config/config.template/contents instead of config/contents

p4block commented 3 years ago

I just tested and it actually does than when using unix tools (not gnu coreutils cp). Good catch, I actually have rsync symlinked to cp and was able to experience the issue first hand. However, it could be argued that most people have a modern cp that doesn't work that way.

I'm all for unix compatibility though...

EDIT: it also turns out I was looking at the wrong commit

p4block commented 3 years ago

So I've been playing a while with it, using /bin/cp -R config.template/* config will cause an error if the config folder doesn't exist. (Which will not happen if /bin/cp -R config.template/ config is used instead.)

Subsequent uses of the non-* version however will copy the folder inside instead of the contents.

mkdir config && /bin/cp -R config.template/* config? Let's not think about subfolders

Rsync behavior is an entirely different beast because you need -a to not create an additional level, which I use, but -R disables it (aha)

A good version that always works is rsync -a config.template/ config

BlackYps commented 3 years ago

I'm not a linux pro. I don't have any strong opinion on what the best solution is. I just noticed that it doesn't work with the standard linux cp command used by Manjaro that I have not tinkered with in any way, and we should fix that.

Brutus5000 commented 3 years ago

But seriously: The instruction here is for freshly cloned repos, there is no existing folder and the command works.

p4block commented 3 years ago

Exactly, there's nothing wrong with the command as-is. @BlackYps brought up the issue with unintended behavior if the folder has already been copied, but honestly cp isn't the tool for that. One cannot safely assume what the user wants to do in that case, it requires a well flagged rsync. Delete and recopy? Copy on top but keep new files? Copy only modified? No way to put the "perfect command" in the instructions.

BlackYps commented 3 years ago

Maybe you are overthinking this. What I want to do is update the config folder after I pulled an update. We could also make a new section in the readme, if you don't want to change the command for the initial setup. I just wanted to update the database and ran into the issue that I had to update the config folder and the command I tried didn't work. We have a section to update the db, so imho it should contain the whole process to successfully do that.

Sheikah45 commented 3 years ago

But the section you changed is for initial setup of faf-stack when config doesn't exist so will then error out.

Brutus5000 commented 3 years ago

@BlackYps Copying over the folder would loose all your changes, which is not recommended either. If you don't care about the old configs you can delete them first. There is no right or wrong way to describe it. It depends.

Brutus5000 commented 3 years ago

If you want to keep the default config you could as well just create a symlink from config.template to config

BlackYps commented 3 years ago

I updated the PR to not change the initial setup command but instead add some more explanation to the update section. The cp command asks for confirmation for overwrites, so people should not accidentaly lose their local changes.

Brutus5000 commented 3 years ago

But your comment is not related to udpating database schemas at all. It applies to any service which may have received a config change :shrug:

BlackYps commented 3 years ago

Maybe I should explain my situation a bit more: I am using faf stack to be able to run a local server instance for testing. Sometimes I need to update the db. So I looked into the readme and ran the command to update the database schemas. That failed, because it was missing configuration files. I figured that they were changed in the meantime so I tried to copy the configuration template files again. The database migration still failed and then I found out that the configuration files have been copied in the wrong folder. So I wanted to improve the documentation, so it explains better how to actually successfully migrate the db when you are largely unaware how the faf stack functions, because you are actually a server dev that just uses faf stack as a tool. That's why I was ignorant to other use cases, like generally updating the config, without a db migration, because I have never needed that. I will put them into a separate section.

Sheikah45 commented 3 years ago

If this is all just about updating the db wouldn't it be better to add it to the updating the db section in the db repository?

BlackYps commented 3 years ago

Actually I think you are right. At the very least the db readme also needs more explanation. If you think you don't need any changes in this readme here you can close this PR.

Brutus5000 commented 3 years ago

The Readme does need an overhaul in general and updating the configuration / keeping it up to date is a struggle I am facing myself a lot (e.g. on the test servier). The problem is that there is no simple solution in cases where you want to override the config for testing.