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

rename Movefile to movefile.yaml by default #424

Merged
merged 1 commit into from
Nov 16, 2017

Conversation

alessandro-fazzi
Copy link
Member

No description provided.

@alessandro-fazzi
Copy link
Member Author

I'm having some problems because Dir[] method's case sensitiveness is system dependant.

@alessandro-fazzi
Copy link
Member Author

In reply to @nlemoine 's #46 (comment)

I'm ok with the .yml, but not with the capital letter: I've always seen the "first capital if no extension" convention and I'd really like to be as conventional as possible.

I'm improving the code to better handle variations.

@nlemoine
Copy link
Contributor

I'm ok with the .yml, but not with the capital letter: I've always seen the "first capital if no extension" convention and I'd really like to be as conventional as possible.

Didn't think about this, you're right!

@alessandro-fazzi
Copy link
Member Author

That is fucking green! 🎉

Doing some other live tests

@alessandro-fazzi
Copy link
Member Author

I've tested and it works well. I've also squashed all the commits into 1 clear commit to not pollute repo history. Wiki is also updated here and we have a new post-installation message mentioning the new feature.

1 thing to spot out: no problem when you start out a new project, but if you update an existing one, you have to update the exclude section in the movefile.yml or it will be pushed remotely when pushing or deleted when pulling.

When starting clean, the generated movefile.yml handles it all on itself.

@alessandro-fazzi alessandro-fazzi merged commit 67a8ac6 into master Nov 16, 2017
@alessandro-fazzi alessandro-fazzi deleted the movefile_dot_yaml branch November 16, 2017 11:15
@nlemoine
Copy link
Contributor

Great! 👍

1 thing to spot out: no problem when you start out a new project, but if you update an existing one, you have to update the exclude section in the movefile.yml or it will be pushed remotely when pushing or deleted when pulling.

I think movefile (in all variations) should probably be excluded by default.

  • First because I don't see any reason where someone would need to upload this file on an environment
  • But mostly because YAML is a flat file that can be easily accessed if some users don't put it under their doc root directory. And this name change introduces the possibility that the file could be uploaded without user knowing it, introducing some potentially serious security issues (published database & ssh credentials)

What do you think?

@alessandro-fazzi
Copy link
Member Author

That's what I intended w/ "no problem when you start out a new project" ;)

Take a look at the template here

@alessandro-fazzi
Copy link
Member Author

@nlemoine I've released the updated gem, so you can also try to do a wordmove init and see if the generated movefile fits well

@nlemoine
Copy link
Contributor

no problem when you start out a new project

Sure thing.

But you have quite a large community already using Wordmove. Meaning they already have a Movefile in their project that won't exclude movefile.yml from the sync process. And I'm pretty sure that most of them won't have look at the changelog and even notice the change when updating Wordmove to 2.2.0.

Thus, they could accidentally publish a Movefile that could be easily exploited if placed at the doc root. I'm concerned by the fact that it could potentially lead to serious troubles (some people could scrap some WP sites and look for Movefiles, WP is a famous target for all kind of exploits).

Besides, IMHO, the only fact that uploading a file that contains nearly all possible credentials and is certainly useless on a server in every case is a strong enough argument. Moreover, It would strip a few lines from the movefile.yml making it cleaner.

@nlemoine
Copy link
Contributor

nlemoine commented Nov 16, 2017

Ok, just realized that if you have a Movefile, chances are that the Movefile is part of the exclude hash... 🐌

However, I still think the last part of my comment remains relevant (publishing a flat file containing most credentials).

@alessandro-fazzi
Copy link
Member Author

alessandro-fazzi commented Nov 17, 2017

Well,

let's start from a philosophical point of view: Wordmove is not a plugin, but it's a tool. Just like with a hammer you have to know to hit the nail and keep your fingers safe, I think that using Wordmove you have to be aware of a lot of critical things: you may need to know how rsync mirroring works, what advanced options it offers, same thing for mysqldump, you have to know your environment (executables in PATH, etc) and so on and so forth.

Wordmove does not aim to prevent all the dangerous scenarios by itself for 2 main reasons: 1) it is so flexible that it fits too many scenarios and so is not possible to programmatically anticipate all of them 2) it would become too hard to maintain and to develop - and believe me: it's really hard.

That said: I really like the way you approach to the problem, so, please, let's consider these 2 scenarios in our discussion.

I've just updated to version 2.2.0 and I have an existent project

  • I have a Movefile
  • it is excluded already
  • I can continue without modifications. Just be happy.
  • If I follow the releases here on GH and I'm aware of the new feature and I can rename my Movefile to movefile.yml. This should trigger me to update exclusions. If I won't do it, when I will first do wordmove pull -w I'll delete my movefile.yml. If I'll wordmove push -w I'll upload my movefile.yml actually.

I've just updated to version 2.2.0 and I start a new project

  • I do wordmove init
  • I have a movefile.yml
  • I have my butt already covered. Just be happy.

In order to avoid the last point of the first scenario I'd have to implement something like:

  • be conscious of what I'm doing. :P Update the "exclude" section while working. Wordmove is just using rsync on my behalf, after all.
  • hardcode exclusion of all flavour of supported movefile's naming in the rsync command despite user configuration

That said I can't hardcode exclusion of custom named movefiles - they are supported by wordmove - nor the exclusion of the (i-see-what-u-did-there) Movefile_bak/_old/_prev.

So we can write a little feature supporting the officials naming, but then we'll have the responsibility to document that we cannot do black magic if you choose to adopt supported custom names. This is a trade off in my opinion.

I'd take some time to think about this one and choose between improving the documentation on the main README - in order to have more conscious users - and implement a guard feature.

@nlemoine
Copy link
Contributor

Thanks for the very detailed reply and arguments.

I think that using Wordmove you have to be aware of a lot of critical things: you may need to know how rsync mirroring works, what advanced options it offers, same thing for mysqldump, you have to know your environment

I don't totally agree with this and I guess many issues you probably deal with every week on GH prove it (especially with the --delete feature in rsync). Using a tool doesn't mean you're completely aware of what's going on under the hood. I drive a car and don't know in details what makes it work. It's probably exaggerated but the point is: some users tend to take a tool, push a button and expect things to just "work".

Anyway, this a more global view that's a bit outside the scope of this discussion. I totally understand your point of view and think you're right about the exclusion policy of this tool. After all, Wordmove is not targeting end users but developers who should be aware that with great power comes great responsibility 😄

An explicit warning/disclaimer in README about what can go wrong when using Wordmove (deleting files, publishing unwanted files, etc.) is probably the right direction on this.

This is more a documentation effort to encourage users to adopt best practices like password less authentication, protecting sensible files access, etc. With the coming wp-cli support, I guess database credentials could also disappear from movefile making it less sensible.

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.

None yet

2 participants