-
Notifications
You must be signed in to change notification settings - Fork 10
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
Added deployTags option #4
base: master
Are you sure you want to change the base?
Conversation
…o create and push new tags when deploying
…se correct heroku push options)
I'm cleaning this up now for merging and I'm writing some tests (if other people are using this, it's probably about time...). One thing that scares me is that you git push -f for tags (the -f is the scary bit). Could you explain the workflow you're using there? The part that gets me is you create a tag first (so I'm assuming the code is new), then you force the push (which means you are expecting to remove commits - so I'm assuming you move back to old code). Those seem at odds. Understanding your workflow would help me out on this one. Cheers! |
Hey! So the use case I'm working with is that I'd like to auto create & deploy tags in a continuous deployment scenario - so, the tags don't necessarily originate from the same place. For instance, I could be working on a branch, and deploy directly from there (without merging to master). That scenario would cause git to come back with an error, as heroku could have been updated from a different branch. The other (probably more common) use case is in the case of rollbacks - if I want to deploy a previous tag over the existing one, it will never work unless it's forced. You definitely have a good point about not attempting to create a new tag in a rollback scenario - I'll add some code to prevent tag creation when the tag already exists to make this clearer. I agree though that -f is a little scary, but I think the user-intent is to deploy whatever they're looking at, it would be up to the user to make sure they have their test runner tasks before this task. The alternative would be to make this optional, and return an error if heroku refuses the push unless you've added Thanks! Jesse |
… an error if forcePush is not set to true
Ok, so that commit will only use -f if forcePush is set to true. In addition, it will fail if the tag that you're trying to create already exists, unless forcePush is set. |
Ok so from that and my own experiences the common usages are:
You also mentioned deploying from a branch that you don't want merged, but rather you want to
I think it's best to ignore that usage for now and handle it separately. Let me know if you disagree. So the other 2 cases... 1If you specify deployBranch, it will do the merge to your deploy branch. If you also specify deployTag, it will tag the merge commit with that tag name. If you specify neither, the default is still
Corresponds to:
2If you specify deployTag only, it will deploy an existing tag to heroku using push -f by default. Default options are:
Corresponds to:
Lemme know if you see any weirdness in there. Also if you are touching this again, please fix up JSHint errors. |
(thumbsup) and since we're now forcing tag creation as well as push, maybe
|
Sounds good! I'll add use strict and lint all the files, and add replace forcePush with force, as well as make pushTagTo an array. I think that will make everything work as you mentioned in the above reply. |
Awesome!
|
Cool, so writing this stuff, looks like 2 things are necessary to make it work:
I'll shoot an update with the rest in a few minutes. |
Makes perfect sense. I'll take a look and merge after work tonight. (it's
|
Ok, updates are in. I'm actively using this at work, so it's pretty well manually tested. |
} | ||
|
||
function tagExists(tag,next){ | ||
exec('git tag | grep ' + tag,function(err,stdout,stderr){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using grep
is going to stop this from working on Windows. Also would fail if they were tagging 1.0
and 0.1.0
already existed. I think this would work instead (I haven't tested though):
function escapeRE(s) {
return s.replace(/[\-\/\\^$*+?.()|[\]{}]/g, '\\$&');
}
// ...
exec('git tag', function(err,stdout, stderr) {
var tagRE = new RegExp('^\\s*' + escapeRE(tag) + '\\s*$');
var isListed = stdout.split('\n').some(tagRE.test.bind(tagRE));
next(isListed);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call - thanks!
On Feb 14, 2013, at 3:26 AM, hitsthings [email protected] wrote:
In tasks/lib/heroku-deploy.js:
+}
+
+function allOutput(proc, next) {
- var out = '';
- proc.stdout.on('data', function(data) { out += data; });
- proc.stdout.on('end', function() {
- next(null, out);
- });
+}
+
+function trim(str){- return str.replace(/^\s+/,'').replace(/\s+$/,'');
+}
+function tagExists(tag,next){
- exec('git tag | grep ' + tag,function(err,stdout,stderr){
Using grep is going to stop this from working on Windows. Also would fail
if they were tagging 1.0 and 0.1.0 already existed. I think this would work
instead (I haven't tested though):
function escapeRE(s) {
return s.replace(/[-/^$+?.()|[]{}]/g, '$&');
}
// ...
exec('git tag', function(err,stdout, stderr) {
var tagRE = new RegExp('^\s' + escapeRE(tag) + '\s*$');
var isListed = stdout.split('\n').some(tagRE.test.bind(tagRE));
next(isListed);
});
—
Reply to this email directly or view it on
GitHubhttps://github.com//pull/4/files#r3011521.
I'll do the rest of the cleanup, but I'm going to take a couple days, so I hope you're not in a rush to get it on npm for any reason. I can release a milestone version pre-cleanup if you need. Cheers and thanks again for sharing! |
No hurry! I'm pointing directly to my fork for now, I'll switch back to On Feb 14, 2013, at 3:31 AM, hitsthings [email protected] wrote: I'll do the rest of the cleanup, but I'm going to take a couple days, so I Cheers and thanks again for sharing! — |
Added new options & documentation to support auto creation of tags, and optionally pushing them to origin. From new docs: