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

Context.config reading should be branch-aware #264

Closed
jbjonesjr opened this Issue Sep 25, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@jbjonesjr
Contributor

jbjonesjr commented Sep 25, 2017

Currently context.config does not pass a ref, so the read defaults to the master branch.

probot/lib/context.js

Lines 89 to 92 in a86d1f7

const params = this.repo({path: path.join('.github', fileName)})
try {
const res = await this.github.repos.getContent(params)

Related to source:
https://github.com/mikedeboer/node-github/blob/79619e8e070112f245a6f29fb091c85b7e1d0556/lib/routes.json#L5820-L5841

(from the docs: The String name of the Commit/Branch/Tag. Defaults to master. )

The config method should support an additional param to allow this configuration

@JasonEtco

This comment has been minimized.

Show comment
Hide comment
@JasonEtco

JasonEtco Sep 27, 2017

Member

@jbjonesjr that's a great idea!

We should figure out how to deal with the params then. Currently, the second param of context.config() is an optional defaultConfig object; when you don't want to include it, what should it look like?

// Pass null
const cfg = await context.config('config.yml', null, 'branch-name');

// Pass an empty object
const cfg = await context.config('config.yml', {}, 'branch-name');

// Check if the second param is a string
// If it is then thats the branch and the third param is defaultConfig
const cfg = await context.config('config.yml', 'branch-name');

Or, do we do it the other way around and make the second param be the branch all the time?

async config(filename, branchname, defaultConfig) {}

const cfg = await context.config('config.yml', 'branch-name', { foo: 'bar' });
const cfg = await context.config('config.yml', 'branch-name');
Member

JasonEtco commented Sep 27, 2017

@jbjonesjr that's a great idea!

We should figure out how to deal with the params then. Currently, the second param of context.config() is an optional defaultConfig object; when you don't want to include it, what should it look like?

// Pass null
const cfg = await context.config('config.yml', null, 'branch-name');

// Pass an empty object
const cfg = await context.config('config.yml', {}, 'branch-name');

// Check if the second param is a string
// If it is then thats the branch and the third param is defaultConfig
const cfg = await context.config('config.yml', 'branch-name');

Or, do we do it the other way around and make the second param be the branch all the time?

async config(filename, branchname, defaultConfig) {}

const cfg = await context.config('config.yml', 'branch-name', { foo: 'bar' });
const cfg = await context.config('config.yml', 'branch-name');
@jbjonesjr

This comment has been minimized.

Show comment
Hide comment
@jbjonesjr

jbjonesjr Sep 28, 2017

Contributor

@JasonEtco my thinking was:

  • if context already is related to a branch (aka, it was a push, commit, or similar action), that is used by default.
  • if the context does not relate to a branch (issue.opened, etc), defaults to master
  • can always override this default behavior with a param passed to context.config
Contributor

jbjonesjr commented Sep 28, 2017

@JasonEtco my thinking was:

  • if context already is related to a branch (aka, it was a push, commit, or similar action), that is used by default.
  • if the context does not relate to a branch (issue.opened, etc), defaults to master
  • can always override this default behavior with a param passed to context.config
@SarasArya

This comment has been minimized.

Show comment
Hide comment
@SarasArya

SarasArya Oct 7, 2017

Is anyone working on this? I would like to work on this

SarasArya commented Oct 7, 2017

Is anyone working on this? I would like to work on this

@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot Dec 6, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale bot commented Dec 6, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Dec 6, 2017

@stale stale bot closed this Dec 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment