feat: Auto detect commit sha, add shorthand -r flag #119
Conversation
YOU54F
force-pushed
the
auto_detect_commit
branch
from
dcf6472
to
2e6d75d
Compare
|
Ping me when you're happy with this. |
| def consumer_app_version | ||
| require 'pact_broker/client/git' | ||
| if options.consumer_app_version.blank? && options.auto_detect_version_properties | ||
| PactBroker::Client::Git.commit(raise_error: explict_auto_detect_version_properties) |
There was a problem hiding this comment.
Ok, all this "auto detect vs explicit auto detect" logic was because I started by auto detecting the branch, and I was going to make it auto detect the branch by default, but have it not raise errors if it couldn't find the branch env var if it was the "default auto detect", but for it to raise errors if the user had explicitly said "auto detect", because I wanted to turn it on without breaking anyone's builds. I'm not sure if that logic needs to be applied to the consumer version, because the consumer version is a required property, but the branch wasn't.
There was a problem hiding this comment.
"auto detect vs explicit auto detect"
Logic was because I started by auto detecting the branch, and
- I was going to make it auto detect the branch by default
- don't raise errors if it couldn't find the branch env var if it was the "default auto detect"
- raise errors if the user had explicitly said "auto detect",
I wanted to turn it on without breaking anyone's builds.
I'm not sure if that logic needs to be applied to the consumer version, because the consumer version is a required property, but the branch wasn't.
Edited to read it a bit better, I had a look last eve, and I think I am confused. I'll push up some changes
There was a problem hiding this comment.
Just make this (raise_error: true)
There was a problem hiding this comment.
sorry I did pick this up this morning to try and wrap it up but got pulled away on various threads. thanks for the extra reviews comments :)
|
Hiya Beth, sorry for late reply, haven’t had the headspace to jump back on this at the moment, but will make the logic less complex and meet your wish list. i’ll be sure to give you a holla so we can tee this up with some docco changes to recommended workflows ( and box off the path to nirvana updates for branches / envs ) |
|
Can you have a poke around and see if you can do the same for the build URL? |
yeah sure thing, I already know some of them from previous work on a slack reporter for cypress |
| def consumer_app_version | ||
| require 'pact_broker/client/git' | ||
| if options.consumer_app_version.blank? && options.auto_detect_version_properties | ||
| PactBroker::Client::Git.commit(raise_error: explict_auto_detect_version_properties) |
There was a problem hiding this comment.
Just make this (raise_error: true)
YOU54F
force-pushed
the
auto_detect_commit
branch
from
630ab6a
to
bcce5a8
Compare
YOU54F
force-pushed
the
auto_detect_commit
branch
from
1c49e40
to
98fd08f
Compare
|
Should be good for review again now, have moved out the DX instructions to #143 |
|
This must be the longest running most painful PR ever. My sincere apologies.
|
|
Thank you for your patience and perseverance with this @YOU54F! We got there in the end 😆 |
|
yay and no problem @bethesque - thanks for the guidance as always and also thank you for wrapping up the last bits 🥰 |
|
WOOT - better later than never hey! |
|
Unfortunately, it's not going to be much use to the people using the docker image, because you have to map the env vars deliberately to make use of it. But it will help anyone using the standalone. |
|
good point Polly the party pooper 😂 We can provide guidance to users to either mount their env vars into the container, so they will be picked up |
resolves #100
--auto-detect-version-propertiespick up both the git commit, and git branch, from available env known CI env vars , or git commands if in a git repo.-rflagWould love to make default behaviour, but in lieu of that I used the shorthand flag for
-rfor recommended :)Should fit in nicely with our re-work of the path to nirvana