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

Sync settings after App installation #178

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mvegter
Copy link
Contributor

@mvegter mvegter commented Nov 16, 2019

Fixes #1

Implementation has overlap with #179

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved

robot.on('installation_repositories.added', async context => {
const { payload } = context
const { repositories_added: repositories, installation } = payload
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that this is very similar to the installation event. Have you considered doing something like repositories = payload.repositories | payload.repositories_added and just combine the two events.

@travi
Copy link
Member

travi commented Nov 21, 2019

am i correct in understanding that this would directly update the file in the default branch? if so, it seems dangerous to do this without submitting the changes as a PR to give the repository maintainer a change to review and accept the changes. if a project already has the settings file, it's likely that there is a reason for it to include the contents that it does, even if it should be supplemented with additional details.

@mvegter
Copy link
Contributor Author

mvegter commented Nov 24, 2019

What do you mean with directly updating the settings file?

The flow this results in:

  • Probot settings is installed by user (all or selected repos)
  • Probot will loop the previous repos and if it finds a settings file it will sync it
  • If the user installs additional repos it will repeat this loop for the new repos.

@travi
Copy link
Member

travi commented Dec 4, 2019

sorry, i definitely misunderstood the goal. i was thinking about the opposite situation where a repo without an existing file would have one created by pulling the existing settings for that repo through the api and written to the config file. i think there was an issue for that at one point, so i think i made the wrong assumption before reading deeply enough.

with a better understanding now, i do see high value to this. i have been really busy lately, but will try to look through your PR soon and give feedback soon.

@mvegter mvegter force-pushed the feature/app-installation-sync branch from e36f4bd to 5d17e60 Compare December 4, 2019 09:47
@mvegter mvegter force-pushed the feature/app-installation-sync branch from 5d17e60 to c3cdb68 Compare December 8, 2019 17:33
@stale
Copy link

stale bot commented Mar 7, 2020

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

@stale stale bot added the wontfix label Mar 7, 2020
@travi travi added the pinned label Mar 7, 2020
@stale stale bot removed the wontfix label Mar 7, 2020
@travi
Copy link
Member

travi commented Mar 7, 2020

sorry that i still havent gotten to this, but it is still on my radar

pamelasarkisyan pushed a commit to pamelasarkisyan/settings that referenced this pull request Jul 29, 2021
pamelasarkisyan referenced this pull request in pamelasarkisyan/settings Jul 30, 2021
@rogerluan
Copy link

Maybe ping @travi ? 😊

@jftanner
Copy link

Is there anything blocking this PR @travi?
This (or another implementation for #1) would significantly improve the security stance when using this app.

Currently, an org admin configuring a new repo needs to do two tasks in order:

  1. Install the app in the repo
  2. Push/merge a commit with .github/settings.yml with the required branch protection rules

If they do those in the wrong order, then it's very easy for them to be under the mistaken impression that the repo has been configured safely. This leaves the repo significantly vulnerable until either the settings are updated or an admin notices the missing rules.

That condition is especially likely when creating repos from a template, as the template is likely to have the settings file already.

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

Successfully merging this pull request may close these issues.

Sync settings after installing the integration
6 participants