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

Use date('YmdHis') instead of an integer value to represent a release. #174

Merged

Conversation

tomzx
Copy link
Contributor

@tomzx tomzx commented Feb 13, 2015

No description provided.

@antonmedv
Copy link
Member

Well, why use YmdHis instead ща number of release? You can always get the time of release by modify time of dir.

And i think number for releases is more readable for humans:

current ⇢ 3
releases 
├— 1
├— 2
├— 3

@jensk
Copy link

jensk commented Feb 13, 2015

+1 for YmdHis

@tomzx
Copy link
Contributor Author

tomzx commented Feb 13, 2015

I agree that numbers are more readable and that ls -l would list the modification time of the folder, but the website (http://deployer.org/docs/recipes) lists that the format is YmdHis. Furthermore, the various deployment tools I've seen (capistrano, rocketeers, magallanes) seem to prefer it over numbered release.

I think it might be that if you are releasing to many servers at once and that the release name is YmdHis from the deploying machine, then all remote servers will have this same folder name, while a numbered release may not (if servers are added with time and their release number do not match). Then it is not possible to figure out which release matches which on the remote server from a simple file listing.

On the other hand though, it would still be possible to have some sort of centralized numbering system so that any deployment to many server deploys to the same release folder number.

@KennedyTedesco
Copy link
Contributor

👍

@antonmedv
Copy link
Member

@tomzx i agree. But what if we get differences in last s?

@tomzx
Copy link
Contributor Author

tomzx commented Feb 13, 2015

@Elfet, that's why I said we get the timestamp/datetime from the deployment machine (aka your desktop/laptop) not the deployed "at" machine. This value is exactly the same for every deployment on every machine.

Some simple like this in your recipe file:

$release = date('YmdHis');

task('deploy:release', function () use ($release) {
    $releasePath = "{deploy_path}/releases/$release";

    run("mkdir $releasePath");

    run("cd {deploy_path} && if [ -e release ]; then rm release; fi");

    run("ln -s $releasePath {deploy_path}/release");
})->desc('Prepare release');

if (!empty($releases)) {
$release = (int)$releases[0] + 1;
}
$release = date('YmdHis');
Copy link
Member

Choose a reason for hiding this comment

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

In current implementation of this feature in PR we get this: then deploy process in sequence on every server we get +20 sec, +40 sec, +60 sec for every release on every server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. I can fix it if we go forward with this change.

@antonmedv
Copy link
Member

So what everybody think? @gordalina?

@gordalina
Copy link
Contributor

Both approaches are valid. I would go for YmdHis, because it's the standard in deployment tools.
Incremental release number could be used if a parameter is set: e.g.: set('release_naming', 'incremental'); whereas the default would be 'timestamp'.

@tomzx
Copy link
Contributor Author

tomzx commented Feb 13, 2015

@gordalina Agreed. I thought about the same thing, but I decided against suggesting it for the sake of keeping the system simple (as we'd have to handle both case and consider them when making any changes in the future).

@gordalina
Copy link
Contributor

@tomzx I favor convention over configuration. The configuration option is a solution in case @Elfet has a strong opinion about incremental release names.

antonmedv pushed a commit that referenced this pull request Feb 15, 2015
…older-name

Use date('YmdHis') instead of an integer value to represent a release.
@antonmedv antonmedv merged commit 237727d into deployphp:master Feb 15, 2015
@antonmedv
Copy link
Member

Done!

@tomzx
Copy link
Contributor Author

tomzx commented Feb 15, 2015

@Elfet Looks like you went a bit fast there. The date call was supposed to be done outside of the task so that all servers would use the same timestamp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants