Add self-update command #84
Conversation
| { | ||
| $this | ||
| ->setName('self-update') | ||
| ->setAliases(array('selfupdate')) |
There was a problem hiding this comment.
i don't think we need the alias
There was a problem hiding this comment.
At least, having this alias would be inline what Composer and the Symfony Installer offer (thus people might be used to use selfupdate instead of self-update).
|
thank you soo much, i added a bunch of small comments. |
|
@amansilla i would like to have 100% coverage for the project. please let me know if you also want to work on the tests. i didn't tried it but i think it could be possible to use symfony's filesystem component in order to avoid mocking the filesytem. |
| rename($tmpFilename, $localFilename); | ||
| $output->writeln('<info>Deprac was successfully updated.</info>'); | ||
| } | ||
| } |
There was a problem hiding this comment.
we should also make the phar file executable (chmod +x)
There was a problem hiding this comment.
Do you think the phar file permissions being executed (so it already has execution permission) could be changed by the rename somehow? Otherwise I can't understand why that is necessary, just curiosity 😉.
There was a problem hiding this comment.
you're right, no need for chmod +x
There was a problem hiding this comment.
Maybe I wasn't right 😂.
After a try, I experienced that new/renamed files in linux system are never going to have execution right independently of the umask set for the directory. Adding +x for all users would be insecure so I'd propose to read the original file permissions before the rename for setting it later to the updated file. That way the original file permissions stay untouched.
amansilla
force-pushed
the
self-update-command
branch
2 times, most recently
from
9b96b8f to
23ef1fd
Compare
|
@slde-flash I've updated the command with your suggestions and added the documentation, so now it should be good enough. About the tests, I've being already thinking about it but I guess current the code is not fully testable without a previous refactoring. I guess we could use the filesystem component for the file handling and maybe guzzle for fetching the remote phar. |
| $output->writeln('<error>Could not write deptrac.phar</error>'); | ||
|
|
||
| return 1; | ||
| } else { |
There was a problem hiding this comment.
you can remove the else statement here
|
i am 👍 using guzzle and the filesystem component. |
amansilla
force-pushed
the
self-update-command
branch
from
23ef1fd to
31b9698
Compare
|
What about using https://github.com/padraic/phar-updater here? |
|
I think it's a good option and simplifies the command, they only problem I see, this requires to maintain a new |
| @@ -3,18 +3,19 @@ | |||
| "license": "MIT", | |||
| "require": { | |||
| "php": ">=5.5.9", | |||
| "graphp/graphviz": "0.2.*", | |||
There was a problem hiding this comment.
can you try to not reorder the dependencies?
There was a problem hiding this comment.
For sure, it'll be fixed on next update. I just like and am used to order the dependencies, I find it more readeable.
There was a problem hiding this comment.
feel free to create a PR that just reorders the dependencies =)
There was a problem hiding this comment.
Ok, now I got your point of view. ;)
|
@amansilla would you prefer moving to https://github.com/padraic/phar-updater if i would maintain the deptrac.version file? |
|
@slde-flash I'll to refactor this again, for using the phar updater as it seems to be easy to use and it's integration will simplify the testing as well. In case I find any new problem with it, I'll let you know. |
|
@amansilla thank you sooo much :) |
amansilla
force-pushed
the
self-update-command
branch
from
31b9698 to
870d483
Compare
|
@slde-flash I refactored the code for using the phar updater. Before I try to add some tests, could you please review the code? |
|
|
||
| class DeptracStrategy extends ShaStrategy | ||
| { | ||
| const PHAR_URL = 'http://get.sensiolabs.de/deptrac.phar'; |
There was a problem hiding this comment.
please dont use CONST (because const is "public"), i am fine with inlining this or private static :)
There was a problem hiding this comment.
The URL was inline added for both ;).
|
@amansilla looks awesome. i am going to play with this pr tonight, if something is missing, i'll take over :) |
amansilla
force-pushed
the
self-update-command
branch
from
870d483 to
2b6a23f
Compare
amansilla
force-pushed
the
self-update-command
branch
from
2b6a23f to
028b46b
Compare
amansilla
force-pushed
the
self-update-command
branch
from
028b46b to
2a6bfa5
Compare
This is a proposal for the self-update command #13 to be used as base although it needs probably some modifications.
Since the latest
deptrac.pharis hosted inhttp://get.sensiolabs.de/deptrac.phar, I thought the following location can be used to indicate its versionhttp://get.sensiolabs.de/deptrac.version(currently not existing and should be created by SensionLabs as they admin the continuous delivery process I guess), which will be used in the code to fetch the latest version and compare it with the running one.@slde-flash Let me know, what do you think about this or if you need a more sophisticated solution 😃.