Skip to content

Commit

Permalink
feat: use GIT_BRANCH for Jenkins, close #4
Browse files Browse the repository at this point in the history
  • Loading branch information
@bahmutov
bahmutov committed Jul 12, 2018
1 parent 9e1dd92 commit 4f95da1
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion src/utils.js
View file
Expand Up @@ -17,7 +17,8 @@ function getBranch (pathToRepo) {
'CIRCLE_BRANCH',
'TRAVIS_BRANCH',
'BUILDKITE_BRANCH',
'CI_BRANCH'
'CI_BRANCH',
'GIT_BRANCH' // on Jenkins
]
const ciBranch = firstFoundValue(ciNames, process.env)
if (ciBranch) {
Expand Down

5 comments on commit 4f95da1

@brian-mann
Copy link
Member

Choose a reason for hiding this comment

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

I am like... super not a fan of this utility parsing environment variables. This is incredibly surprising to me. I want it to only find git related things, and nothing more.

We already have our own (wayyyyy) more comprehensive fallbacks in the main repo regarding what environment variables to look for when git does not provide enough information.

I feel like this is overstepping the boundaries of what this utility is supposed to provide. Either this utility needs to have all of the logic of the main repo in the fallbacks or it just needs to handle git.

As it stands, this utility can actually incorrectly report on env vars, which will actually prevent our own more comprehensive fallbacks in the main repo to then parse the environment variables.

@brian-mann
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this more... I would just prefer this repo be renamed to git-info and it stay strictly about parsing git.

@bahmutov
Copy link
Contributor Author

@bahmutov bahmutov commented on 4f95da1 Jul 12, 2018 via email

Choose a reason for hiding this comment

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

@brian-mann
Copy link
Member

Choose a reason for hiding this comment

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

Agree. Yank that out of here and cut a new breaking version. Just keep git specific functions in here and I will be happy. It'll tighten up the scope quite a bit.

Having a different module is an interesting idea - but my first instinct is "no" because we're pulling off the environment variables in a highly controlled, specific structure where only some things are pulled off as opposed to everything.

The problem is that there are oh so many different CI providers and different reasons to use those properties- then the NPM module would still be arbitrary regarding what it pulls off vs not.

Instead, I really think it's okay that's in the primary repo since it makes it really clear what we're looking at and what we're doing with it.

@bahmutov
Copy link
Contributor Author

@bahmutov bahmutov commented on 4f95da1 Jul 12, 2018 via email

Choose a reason for hiding this comment

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

Please sign in to comment.