Add support for env variable for committed_at in Codeship payload
#168
Conversation
Sync with official library
antoniobg
changed the title
committed_at in Codeship payload
There was a problem hiding this comment.
Hi @antoniobg, thanks for taking this on. This direction look good overall and I'm happy to cut a new release on the mainline reporter gem so you don't need to maintain a fork :)
| committed_at = git("log -1 --pretty=format:%ct") | ||
| committed_at.to_i.zero? ? nil : committed_at.to_i | ||
| committed_at.nil? ? nil : committed_at.to_i |
There was a problem hiding this comment.
I believe #git will return an empty string, not nil in this case. If that's right, this should remain unchanged
| } | ||
| end | ||
|
|
||
| def branch_from_git_or_ci | ||
| clean_service_branch || clean_git_branch || "master" | ||
| end | ||
|
|
||
| def committed_at_from_ci_or_git | ||
| committed_at_from_ci || committed_at_from_git |
There was a problem hiding this comment.
I think we should change the order of these so git is preferred over CI
| @@ -34,9 +38,13 @@ def head | |||
| git("log -1 --pretty=format:'%H'") | |||
| end | |||
|
|
|||
| def committed_at | |||
| def committed_at_from_ci | |||
| Ci.service_data[:committed_at] | |||
There was a problem hiding this comment.
I think we ought to add to_i on this to match committed_at_from_git
| @@ -79,5 +79,19 @@ def self.root | |||
| end | |||
| end | |||
|
|
|||
| describe 'committed_at_from_ci_or_git' do | |||
| it 'returns the committed_at from ci' do | |||
| allow(Ci).to receive(:service_data).and_return({committed_at: 'ci-committed_at'}) | |||
There was a problem hiding this comment.
instead of ci-committed_at, WDYT about putting a value such as "1484768698" (result of Time.now.to_i.to_s), which is what we expect codeship to set in this environment variable
| it "does not raise if there's a committed_at in ci_service data" do | ||
| payload[:git][:committed_at] = nil | ||
| payload[:ci_service] = {} | ||
| payload[:ci_service][:committed_at] = Time.now |
There was a problem hiding this comment.
I think this ought to be Time.now.to_i.to_s to reflect the expected format of this value
|
Thank you very much @maxjacobson, I think I've addressed all the issues you mentioned. |
There was a problem hiding this comment.
@antoniobg thanks! will you take a look at the failing build?
| expect(Git.committed_at_from_git_or_ci).to eq Git.send(:committed_at_from_git) | ||
| end | ||
|
|
||
| it 'returns the committed_at from git if there is no ci committed_at' do |
There was a problem hiding this comment.
looks like this scenario description should now be "returns the committed_at from ci if there is no git committed_at"
antoniobg
force-pushed
the
env-var-for-comitted-at
branch
from
90afdbe to
f32a258
Compare
|
@maxjacobson it failed because it was taken the branch name of this PR |
|
No worries and thank you for accepting the PR! |
This is a small follow-up to #168 that I just noticed before cutting the v1.0.5 release
This is a small follow-up to #168 that I just noticed before cutting the v1.0.5 release
We run into this issue #156 where we couldn't get the test coverage to work unless we included the
.gitfolder in the docker image. We've forked the repository and now we pass an environment variable with the date of the last commit, so we don't need to include that.gitfolder. Not sure if it's something you want to include in your repository, but just in case I'm creating the PR.