Skip to content

Add support for Codeship CI#892

Merged
JuanitoFatas merged 11 commits intodanger:masterfrom
ghiculescu:feature/codeship
Sep 14, 2017
Merged

Add support for Codeship CI#892
JuanitoFatas merged 11 commits intodanger:masterfrom
ghiculescu:feature/codeship

Conversation

@ghiculescu
Copy link
Copy Markdown
Contributor

Added codeship as a new CI source. I mostly copied the style of the Semaphore CI source, and used the same logic that danger.js uses in getting a PR from a branch name.

@DangerCI
Copy link
Copy Markdown

DangerCI commented Sep 12, 2017

1 Message
📖 @ghiculescu is not a contributor yet, would you like to join the Danger org?

Generated by 🚫 Danger

# ### CI Setup
#
# For Semaphor you will want to go to the settings page of the project. Inside "Build Settings"
# For Semaphore you will want to go to the settings page of the project. Inside "Build Settings"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Contributor

@JuanitoFatas JuanitoFatas left a comment

Choose a reason for hiding this comment

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

Cool thank you! (only 1 test failure)

$ rspec spec/lib/danger/ci_sources/ci_source_spec.rb:5

@ghiculescu
Copy link
Copy Markdown
Contributor Author

ghiculescu commented Sep 13, 2017

Hey @JuanitoFatas what do you think about ghiculescu/danger@41e8b50...118fadb ?

The problem here is that codeship doesn't expose a PR number in ENV the way most other CIs seem to. The workaround is to look up Github and check for a PR whose head matches the branch we are merging into (code). However, it is unlikely that there would ever be a PR where master is the head (unless you're using a weird git structure).

Are you happy with these sorts of assumptions in Danger? The other idea I had was to do something like this: !Danger::RequestSources::GitHub.new(nil, env).get_pr_from_branch(..., ..., ...).nil? but it seems fairly grimey, and means there will be an extra github API called made for every build.

@deecewan
Copy link
Copy Markdown
Member

for reference @ghiculescu, the extra API call is what we do on danger-js

I don't necessarily think you can assume that master is : a) the 'master' branch, or b) never PRed

@ghiculescu
Copy link
Copy Markdown
Contributor Author

Updated: ghiculescu/danger@41e8b50...8dd90fa

Copy link
Copy Markdown
Contributor

@JuanitoFatas JuanitoFatas left a comment

Choose a reason for hiding this comment

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

Extra API call is ok because they don't expose :(


# this is fairly hacky, see https://github.com/danger/danger/pull/892#issuecomment-329030616 for why
def self.pr_from_env(env, ci_source)
Danger::RequestSources::GitHub.new(nil, env).get_pr_from_branch(env["CI_REPO_NAME"], env["CI_BRANCH"], owner_for_github(env))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need to pass down the ci_source to Danger::RequestSources::GitHub? 🤔
do we need ci_source? 😂

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ci_source gets used by a bunch of stuff eg here and here - i wasn't keen to take on a full refactor.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it's just not required in my particular case - i just need a github client. if you prefer, i can try and find a neater way of writing that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

wait, i misread your comment... fixed in a49fc30

@orta orta mentioned this pull request Sep 13, 2017
@JuanitoFatas JuanitoFatas merged commit eeddfd5 into danger:master Sep 14, 2017
@JuanitoFatas
Copy link
Copy Markdown
Contributor

Yay! 🎉 Thank you for the contributions! It's so awesome!!! <3 <3 <3

@ghiculescu is not a contributor yet, would you like to join the Danger org?

Would you like to join? 🙏 🙇

@ghiculescu ghiculescu deleted the feature/codeship branch September 14, 2017 04:20
@ghiculescu ghiculescu restored the feature/codeship branch September 14, 2017 04:20
@ghiculescu
Copy link
Copy Markdown
Contributor Author

Yeah okay sign me up!

Can you please let me know when the next gem release goes out so I can point our gemfile to it? :D

@orta
Copy link
Copy Markdown
Member

orta commented Sep 14, 2017

Think it already went out 👍

@orta
Copy link
Copy Markdown
Member

orta commented Sep 14, 2017

And the invite has been sent <3 thanks

@ghiculescu ghiculescu deleted the feature/codeship branch February 8, 2019 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants