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

Add support for probot-config #90

Merged
merged 4 commits into from
Jun 30, 2018

Conversation

pholleran
Copy link
Contributor

This PR fixes #89 and implements support for probot/config

I have also reformatted robot.on within index.js to use an arrow function, as per most other probot apps. I've started playing around with changing the module.exports statement to be more in line with other probot apps by doing something like:

const getConfig = require('probot-config')
const Settings = require('./lib/settings')

module.exports = robot => {
  robot.on('push', async context => {
    
  <rest_of_function>

  })
}

but that started causing test failures I have not yet diagnosed. If that's something that makes sense to add to this PR I'm happy to dig in a bit more.

Copy link
Contributor

@JasonEtco JasonEtco left a comment

Choose a reason for hiding this comment

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

Nice, thanks @pholleran. I left two small comments but other than that this looks good to go.

index.js Outdated
const payload = context.payload
const defaultBranch = payload.ref === 'refs/heads/' + payload.repository.default_branch

const config = getConfig(context, 'settings.yml')
console.log(config)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to context.log()? Might as well get the free extra data 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JasonEtco - or we can delete it. That's a remnant of some of my debugging.

index.js Outdated
const payload = context.payload
const defaultBranch = payload.ref === 'refs/heads/' + payload.repository.default_branch

const config = getConfig(context, 'settings.yml')
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be awaited otherwise it might return a pending Promise.

Copy link
Contributor

@JasonEtco JasonEtco left a comment

Choose a reason for hiding this comment

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

🆒 thanks @pholleran!

@JasonEtco
Copy link
Contributor

Oh hm looks like CI is failing now. Let me know if you want help digging into it @pholleran.

@pholleran
Copy link
Contributor Author

@JasonEtco - thanks for offering up some help, I'd appreciate it. I'm pretty new to Jest and am having trouble identifying the underlying problem

@JasonEtco
Copy link
Contributor

Hey @pholleran @jwsloan - sorry about the delay, not sure how four days went by but alas here we are.

Of the two (this PR and #93), I think I prefer this approach. It requires less mocking, and these test look clearer to me. I also tend to avoid calling require within function/methods whenever possible for performance reasons (negligible I know, but habits are habits).

As for the failing tests, You can see the changes in a61b3c7 - I've created a mocked GitHub API client and made robot.auth() return said client. This is more in line with how we typically test Probot Apps (take a look at our testing docs, sparse as they are).

If y'all agree I'll go ahead and merge this. Let me know what you think.

@pholleran
Copy link
Contributor Author

@JasonEtco - thanks for the help with the tests. I'm 🆒 with merging this one - just happy to get the functionality added

@JasonEtco JasonEtco merged commit f775a0f into repository-settings:master Jun 30, 2018
@jwsloan
Copy link
Collaborator

jwsloan commented Jul 2, 2018

Thanks for the explanation, @JasonEtco! And for the link. I'm a Ruby person trying to write reasonable JS, so all info is much appreciated. I'm just glad to see this functionality make it in. My company is almost certain to start using this app now.

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.

add support for probot-config
3 participants