Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calling CircleCI API when CI_PULL_REQUEST is not set #57

Merged
merged 6 commits into from
Feb 12, 2016
Merged

Calling CircleCI API when CI_PULL_REQUEST is not set #57

merged 6 commits into from
Feb 12, 2016

Conversation

marcelofabri
Copy link
Member

  • CHANGELOG entry
  • Create CLI option and environment variable to set circle-token

It should fix #49

@@ -25,6 +25,7 @@ Gem::Specification.new do |spec|
spec.add_runtime_dependency 'colored'
spec.add_runtime_dependency 'nap'
spec.add_runtime_dependency 'octokit'
spec.add_runtime_dependency 'rest-client'
Copy link
Member

Choose a reason for hiding this comment

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

rest client is an extra dependency we don't need, faraday is already in the dependency graph (comes from OckoKit), any chance you can use that? https://github.com/lostisland/faraday

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, I'll take a look at that later. I hadn't used rest-client before also, just seen some gem using it and thought it was good enough.

Copy link
Member

Choose a reason for hiding this comment

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

Ace 👍

@orta
Copy link
Member

orta commented Feb 11, 2016

Everything feels good in the implementation though!

@marcelofabri
Copy link
Member Author

Travis failed because of CHANGELOG, so #56 really worked!

I'm updating it after, so I can update the README at the same time with information about the new parameter.

@marcelofabri
Copy link
Member Author

I don't know if this deserves a mention on README 😁

@orta
Copy link
Member

orta commented Feb 12, 2016

I think it's best that we start building out docs, will try take a stab at this in the morning 👍

@@ -20,13 +21,15 @@ def validate!
def self.options
[
['--base=[master|dev|stable]', 'A branch/tag/commit to use as the base of the diff'],
['--head=[master|dev|stable]', 'A branch/tag/commit to use as the head']
['--head=[master|dev|stable]', 'A branch/tag/commit to use as the head'],
['--circle-ci-token=[token]', 'A Circle CI API token to be used if needed']
Copy link
Member

Choose a reason for hiding this comment

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

IMO, kill this option, only allow setting this form an env var, otherwise you'd be leaking it in the build logs

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense 👍

@marcelofabri marcelofabri changed the title [WIP] Calling CircleCI API when CI_PULL_REQUEST is not set Calling CircleCI API when CI_PULL_REQUEST is not set Feb 12, 2016
@orta
Copy link
Member

orta commented Feb 12, 2016

Yep, yep yep.

orta added a commit that referenced this pull request Feb 12, 2016
Calling CircleCI API when CI_PULL_REQUEST is not set
@orta orta merged commit ebf78ff into danger:master Feb 12, 2016
@orta
Copy link
Member

orta commented Feb 12, 2016

Thanks sir

@marcelofabri marcelofabri deleted the circle-ci-workaround branch February 13, 2016 06:48
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.

Circle CI can sometimes create a commit build for pull requests
2 participants