Fix detached #8
Fix detached #8
Conversation
index.js
Outdated
| @@ -58,7 +58,7 @@ function loadConfig () { | |||
|
|
|||
| function getEnvIdFromBranch () { | |||
| try { | |||
| let branch = sh.exec('git status', { silent: true }).stdout | |||
| var branch = sh.exec('git status', { silent: true }).stdout | |||
There was a problem hiding this comment.
this should be let not var
index.js
Outdated
| var hash = sh.exec('git rev-parse HEAD').stdout | ||
| hash = hash.substring(0, 5) | ||
|
|
||
| return (branch.includes(' ')) ? hash : branch |
There was a problem hiding this comment.
if the branch includes spaces, I would prefer to replace them with dashes than to use the hash. the hash is not user friendly
|
Andy, we're trying to catch the 'dettached head' scenario. Maybe we should
explicitly call that out?
…On Tue, Jun 19, 2018, 10:18 Andy Day ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In index.js
<#8 (comment)>:
> @@ -58,7 +58,7 @@ function loadConfig () {
function getEnvIdFromBranch () {
try {
- let branch = sh.exec('git status', { silent: true }).stdout
+ var branch = sh.exec('git status', { silent: true }).stdout
this should be let not var
------------------------------
In index.js
<#8 (comment)>:
> length: 13,
omission: ''
}), '-')
+
+ var hash = sh.exec('git rev-parse HEAD').stdout
+ hash = hash.substring(0, 5)
+
+ return (branch.includes(' ')) ? hash : branch
if the branch includes spaces, I would prefer to replace them with dashes
than to use the hash. the hash is not user friendly
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#8 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABBw79q7QhQmKsapoeBI3oW_ALUAE6Llks5t-TJPgaJpZM4UsqP3>
.
|
|
@oifland are you doing this for it to work on gitlab ci? If so you can pass in a --env=$CI_COMMIT_REF_NAME ?? Using the hash is problematic because it would mean a new environment for each commit... |
|
Actually, wouldn't it be better if we used "git name-rev HEAD --name-only" instead of trying to parse git status? |
|
Yes, we could do that instead. Right now we get weird errors when there's a
space in the name, so maybe it makes sense to make it clear and just refer
to the env as "detatched-head" and emit a warning to indicate your tip?
…On Tue, Jun 19, 2018, 10:42 Andy Day ***@***.***> wrote:
@oifland <https://github.com/oifland> are you doing this for it to work
on gitlab ci? If so you can pass in a --env=$CI_COMMIT_REF_NAME ?? Using
the hash is problematic because it would mean a new environment for each
commit...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABBw71yC8jt3Gih6qtaChu807DgxGHGxks5t-TgGgaJpZM4UsqP3>
.
|
|
@jollyromp yes -- great callout. @oifland as for spaces in names, we should either replace spaces with "" or with "-" |
|
I added the git name stuff |
|
also branches aren't allowed to have any spaces |
index.js
Outdated
| length: 13, | ||
| omission: '' | ||
| }), '-') | ||
|
|
||
| return branch |
There was a problem hiding this comment.
no need to have the separate variable. you can return at line 63 directly
|
👍 LGTM! One minor comment... |
|
published as 0.10.0 |
|
How does this get uploaded to npm? |
|
manually using npm publish. I use a tool called np to make it a bit easier. |
No description provided.