Skip to content
This repository has been archived by the owner on Sep 14, 2019. It is now read-only.

Support root configs in repos named .github #6

Merged
merged 1 commit into from
May 24, 2018

Conversation

cco3
Copy link
Contributor

@cco3 cco3 commented May 17, 2018

This change adds special support for base repos that are named
.github. In these repositories, configs can be stored in the
.github/ directory as usual, or directly in the root of the
repository.

Issue: #1

@cco3 cco3 force-pushed the dotgithub-root branch 2 times, most recently from 92f129a to 7310c86 Compare May 17, 2018 19:26
Copy link
Contributor

@hiimbex hiimbex left a comment

Choose a reason for hiding this comment

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

this lgtm! cc/ @probot/maintainers for any other thoughts

Copy link
Contributor

@bkeepers bkeepers left a comment

Choose a reason for hiding this comment

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

This looks really good. One comment/question about behavior for the .github repository below, but otherwise 👍

lib/index.js Outdated
@@ -94,7 +94,13 @@ async function getConfig(context, fileName, defaultConfig) {
}

const baseParams = getBaseParams(params, config[BASE_KEY]);
const baseConfig = await loadYaml(context, baseParams);
let baseConfig = await loadYaml(context, baseParams);
if (baseConfig == null && baseParams.repo === '.github') {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the repo is named .github and it is being used with _extends, I think it should skip looking in the .github directory entirely and only look at the root. Any opposition to that?

Copy link
Contributor Author

@cco3 cco3 May 18, 2018

Choose a reason for hiding this comment

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

If we are going to do that, I would suggest always looking at the root (i.e., not supporting the .github/ dir in a .github repo).

The only concern I have about doing that is that some projects (your own? I think you mentioned somewhere) already use a .github repo, and might be confused with an update to probot-config, but this isn't a major issue.

When probot-config is looking at .github automatically without _extends, I think it make sense to only support looking at the root, since there's no precedent set for that at this point.

@gr2m
Copy link
Contributor

gr2m commented May 18, 2018

I’m not sure I understand that right, is this adding support to read configuration for Probot apps to look not only in the current repository in the .github/ folder but additionally also look if a .github repository exists for the owner of the current repository and check there as well?

Update: nevermind me, I missed the "Issue: #1" part 🤦‍♂️ 😫

@cco3
Copy link
Contributor Author

cco3 commented May 18, 2018

@gr2m, this is not looking automatically for a .github repo, but #7 does.

@cco3
Copy link
Contributor Author

cco3 commented May 21, 2018

So @bkeepers, are you fine with dropping support for .github/.github/ and requiring all configs in a .github repo exist relative to the root? I think this would simplify things.

@cco3 cco3 force-pushed the dotgithub-root branch from 7310c86 to 209fd4e Compare May 23, 2018 18:20
This change adds special support for base repos that are named
`.github`.  In these repositories, configs are not stored in the
`.github/` directory as usual, but directly in the root of the
repository.
@cco3 cco3 force-pushed the dotgithub-root branch from 209fd4e to 9e477df Compare May 23, 2018 18:21
@cco3
Copy link
Contributor Author

cco3 commented May 23, 2018

Alright, I updated the change to require configs for .github/ be stored at the root. Among other things, this will reduce the number of requests sent.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants