app icon indicating copy to clipboard operation
app copied to clipboard

Changes in .github repository are not propagated

Open bkeepers opened this issue 6 years ago • 32 comments

Sorry, I didn't get a chance to chime in on https://github.com/probot/settings/pull/90, but it doesn't actually work.

As @jwsloan pointed out in https://github.com/probot/probot-config/issues/10#issuecomment-404657030, we'll need to add special support so that if settings.yml is updated in a .github repository, the changes need to be propagated to all repositories that inherit that config.

cc @pholleran @JasonEtco

bkeepers avatar Jul 12 '18 21:07 bkeepers

@bkeepers - while I agree that would be nice, I am 🆗 with a workflow where the inheriting repo checks for and applies the latest permissions upon the next push.

Here's my understanding of how this "should" work: (recognizing that it does not currently work)

  • changes are applied to settings.yml in a myOrg/.github repository
  • a user makes a push to the myOrg/inheriting-repository
  • the push event triggers a webhook to probot-settings
  • probot-settings checks for updated settings in myOrg/inhereting-repository/.github/settings.yml
    • the _extends: .github property in myOrg/inhereting-repository/.github/settings.yml loads the new config from myOrg/.github/settings.yml
  • probot-settings applies the new settings to myOrg/inhereting-repository

pholleran avatar Jul 13 '18 15:07 pholleran

  • a user makes a push to the myOrg/inheriting-repository

Just to clarify, the push would have to be to the .github/settings.yml file. All other pushes are ignored.

bkeepers avatar Jul 13 '18 17:07 bkeepers

@bkeepers - would it be reasonable, then, to not ignore all other pushes? Would this be lighter touch than trying to propagate changes down to the inheriting repos?

pholleran avatar Jul 13 '18 18:07 pholleran

or, alternatively, would it make sense to explore a "hacky" solution with probot/metadata in which we utilize a closed issue to store the last synchronized commit SHA of myOrg/.github/settings.yml and modify settingsModified to compare the current SHA of myOrg/.github/settings.yml with the one stored in metadata to determine if a change has occurred?

https://github.com/probot/settings/blob/94a7ef31540c056ae2a39f5e28757b079868d61e/index.js#L10-L13

pholleran avatar Jul 13 '18 20:07 pholleran

@bkeepers @pholleran What are your thoughts on this:

  • We add updated_at and updated_by to settings.yml.
  • The settings app is installed on the .github repo as well as all of your nomal repos.
  • The probot-config app exposes what the base repo is.
  • The settings app looks to see if the current push is on the base repo. If it is, it loops through all of the other repos it is installed on (I think it can do that).
  • Within the loop, it puts in a PR to each of those repositories.
  • The PR changes updated_at and sets updated_by to probot-config.
  • The description of the PR includes a link to the triggering commit to .github/settings.yml.
  • The maintainers of each repo decide to accept or not accept that change.

This gives each repo the ability to override a change they don't want to accept.

jwsloan avatar Jul 16 '18 12:07 jwsloan

Actually... Maybe instead of the updated fields, we just keep track of base_commit, which is the latest hash from probot-config. And we update that with a PR.

jwsloan avatar Jul 16 '18 13:07 jwsloan

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

stale[bot] avatar Oct 14 '18 14:10 stale[bot]

I think this would still be very helpful

travi avatar Oct 15 '18 13:10 travi

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

stale[bot] avatar Feb 04 '19 11:02 stale[bot]

Any updates on that workflow @pholleran ? I believe also the settingsModified check makes it imposible do use the opt-out workflow for probot-config.

pecigonzalo avatar Apr 03 '19 19:04 pecigonzalo

That would be a great addition. +1

kairichard avatar Nov 08 '19 21:11 kairichard

This is a big blocker for the use case of applying settings across many repositories.

jpoehnelt avatar Apr 23 '20 17:04 jpoehnelt

it is understood that this is a highly desirable feature, but it also one that needs to be implemented carefully because it has much broader impact than the current behavior. this is high in the priority list, but also needs focused time to do well, which is difficult to come by currently. i have a few features that i plan to get in place before this, in order to make it safer to introduce this behavior, so i cannot promise any timeframe for this being available.

travi avatar Apr 24 '20 03:04 travi

Just cross-posting from #179:

Do you think we could make this configurable in case I don't want to name my .github repository ".github"? The main reason for that is that I have a master repo which has submodules with all the repos in the org. I currently named my "base" repo template, but I'm not married to that. However, I can't name it ".github", because then I can't have a submodule for it in the main stack repo, at least not have it named the same way as the .github repo itself. I could have a submodule "template" that points at the repo ".github", but it would be anomalous compared to everything else.

iphydf avatar Apr 24 '20 21:04 iphydf

I'd like to chime in with our usecase here. I wanted to set up consistent labelling between various of our repos. While GitHub allows setting default labels for new repos now, maintaining an evolving set of labels across repositories is still cumbersome.

Then, I found probot/settings, which sounded like the perfect solution!

  • I created a new repo which only has the label settings
  • I added a .github/settings.yml to the other repos with just a single line, _extends:base-settings
  • I tested ... didn't work ... found this issue here (2 years old now). Oops.

I thought I found the solution to label automation, but this issue basically makes the above workflow kind of useless as-is as I still need to change that single-line yaml on each repo that should have the updated labels.

Besides what others have mentioned in terms of automation (which I understand is complicated), I would already be happy if there was a button/command/action (ideally org level) that just re-runs probot and applies the settings file again, as if a push had happened.

hybridherbst avatar May 30 '20 21:05 hybridherbst

A hacky workaround would be a GitHub Action workflow that, periodically, does a git commit --allow-empty on the child settings file.

alvarosanchez avatar Jul 09 '20 06:07 alvarosanchez

Another possibility would be for this app to react on all push events, not only those affecting the settings file.

So I guess it would be to remove this lines: https://github.com/probot/settings/blob/master/index.js#L19-L27

GitHub
Pull Requests for GitHub repository settings. Contribute to probot/settings development by creating an account on GitHub.

alvarosanchez avatar Jul 09 '20 09:07 alvarosanchez

Whilst not ideal, a periodic check might work- I believe https://github.com/probot/stale has, by default, an hourly scheduler.

GitHub
A GitHub App built with Probot that closes abandoned Issues and Pull Requests after a period of inactivity. - probot/stale

ap0phi5 avatar Jul 29 '20 09:07 ap0phi5

I really like the @alvarosanchez solution, looks like a nice easy change, and just getting it to run on all push events

jay-oswald avatar Nov 16 '20 06:11 jay-oswald

any news on that? i thought Probot/setting will help me achieve single place for repo settings on my org but seems like the duplication of operations still exists.

lechen26 avatar Apr 19 '21 06:04 lechen26

What about searching for repos that _extend a changed config, and then calling syncSettings for each?

IMO, this should maybe be an octokit-plugin-config concern, or a standalone helper. A robust implementation of config(...).extenders would search org the org recursively for _extends: {this config patterns}, and return their paths+context. Then settings could just do config.extenders.forEach(syncSettings)

* Edited to correct my misunderstanding of the _extends keyword.

micimize avatar May 04 '21 01:05 micimize

Is there a spec for how you would want this implemented? I've seen several suggestions for ways to get this behavior but nothing definitive. I know the behavior wanted is "changes in _extends-ed configs are propagated to repos that consume them." I also see that @travi has features they want implemented first to make it safer to do this feature, which makes me think there is already some design in mind for how this would be solved.

With a spec defined this could be something our team can devote some resources to.

wraithgar avatar Sep 23 '21 18:09 wraithgar

there is not a formal spec at this point, but i expect something like the following could solve this fairly well:

  • trigger based on a change to the .github/settings.yml in the .github repo
  • apply changes to the .github repo (this may not always be the goal of defining settings here, but is current behavior that i think is out of scope for getting propogation working)
  • determine which repos in the org extend from the file in the .github repo (i think some of the internals of octoherd could help here)
  • trigger processing of settings for each repo found that extends the settings from the .github repo. i'm not sure if this step should process all of these actions from the same run, or if it would be possible to trigger an event that could trigger a separate run for each of those repos. a few things to consider:
    • if processing from the initial run that was triggered based on the change to the file in the .github repo, octoherd internals could likely help here as well
    • if processing with separate runs, we would need to make sure that processing would not bail because the file from that repo did not change. also, would we lose the ability to report results back to somewhere that an end user could see and debug with?

I also see that @travi has features they want implemented first to make it safer to do this feature

while this is true, i have not managed to make the progress that i've hoped and dont want the shaving of an unkept yak to hold back progress here if we are able to get some momentum behind this.

status checks

mainly, i've wanted to enable reporting status checks that could show failures to help folks debug when things don't go well. i think there could be benefit of reporting a status for each api call that is made based on the contents of the file for the repo that the run was triggered for. i think that gets more complicated when considering a run triggered by the .github repo file. in that case, it probably makes sense to, at most, report a status per repo being updated.

if we could consider incorporating some of that functionality as part of that effort, i would be supportive, but i don't want to hold this up in the absence of that.

testing confidence

the other big consideration i want to keep in front of us is having appropriate test coverage around the behaviors of a feature like this so that maintenance does not become more complex moving forward. if all checks are green, it should be safe to merge a PR into the project. if that is not the case, testing needs improvement. the complexity of testing rises a fair amount when we start considering behavior outside of a single repository, so i just want to make sure that we account for handling that appropriately.

there are still some gaps in coverage that i have not closed since taking over maintenance of the project. i'm not suggesting that we need to close all gaps, but we do need to make sure that no new gaps emerge as a result of this effort. i've also not iterated enough on the integration tests to work out some of the pain points. i normally prefer to use cucumber for that style test, but would probably avoid changing frameworks for now. i am open to refactoring of the existing tests to remove some of the testing pain that still exists as long as we still accomplish the goals of the various testing layers.

summary

does this help clarify the direction that has been on my mind? does it align with your goals? i need to find the time to pull out some better issues for .github repo settings propagation and status checks and reference issues like this to get a plan better organized, but feel free to continue the conversation here until i get things cleaned up a bit.

travi avatar Sep 27 '21 19:09 travi

Only triggering on changes to .github/settings.yml in the .github repo means that is the only file that can be extended upon in a way that will propagate. The _extends keyword can extend from anywhere, not just that file.

wraithgar avatar Sep 27 '21 19:09 wraithgar

The _extends keyword can extend from anywhere, not just that file.

yes and no, depending how you are leveraging the settings app. if using the hosted version extension is limited to that file because we keep our request for permissions pretty narrow. without requesting more permission from all users of the hosted instance, extending from other files would require hosting your own instance.

are you already hosting your own instance? could you describe your approach to extension so that we are clear about the goal? while the hosted instance is limited, i agree that we should consider more flexibility with this implementation.

travi avatar Sep 27 '21 19:09 travi

Our approach is that our org @npm is shared between two teams: the registry and the cli. In order that we don't step on each others' toes with regards to things like issue labels and repo settings we have generated a separate settings file that the cli repos will inherit. It lives outside the .github folder in the .github repo so there is no confusion or collision w/ the registry team.

https://github.com/npm/arborist/blob/main/.github/settings.yml

---
_extends: '.github:npm-cli/settings.yml'

This is working as expected using the current github settings app. The only issue is that we are "locked" in to whatever settings were in that file whenever we last commited the .github/settings.yml file in any given repo.

As I tried to think about possible solutions for this that would be possible to implement, my initial (potentially naive) thoughts were that one of two things could work.

  • a periodic check / recompiling of any repo's .github/settings.yml file to re-apply the config if there is a need (how to determine if there is a need is open to discussion, timestamps of files involved, config diff, let the existing config sync handle things)
  • a way for a github action to trigger the syncSettings code in probot for a given repo. Then the scheduling of updates can be something that can be implemented on a per-repo basis (either through scheduled actions or custom action events)

My thinking went along these lines because the probot-settings app currently only listens to a very limited set of events from the repos it is enabled on, and it seemed like a pretty drastic change to open that up to more events.

GitHub
npm's tree doctor. Contribute to npm/arborist development by creating an account on GitHub.

wraithgar avatar Sep 27 '21 20:09 wraithgar

This is working as expected using the current github settings app.

it looks like i've been assuming, incorrectly, that this wouldn't be possible in the hosted app. however, it does look like it is configured to grant the permissions needed for this. thank you for highlighting this.

one of two things could work

i do believe that those are options, but i don't believe that they would be the ideal approach. while updates on a periodic basis would be better than the very manual approach currently required, i think pushing the changes to the children projects based on updates to parent configs would be a more ideal solution. i would also like to keep the behavior as self-contained in the app as we reasonably can so that users arent required to do a bunch of additional config of actions, etc to get the functionality.

there is an additional benefit to the periodic approach that we shouldnt dismiss, though. there are currently no events that we could listen for when settings are changed through the web ui to become out of sync with what is defined in the settings.yml. having a periodic refresh could help ensure that the window of time where the settings are out of sync is smaller than today, or even with a push approach in place. ideally, i'd prefer there was a way to disable the web config for the settings managed through the config file, or at least have events that could result in this app sendings PRs to update the config based on changes in the ui, but those both seem unrealistic for now.

i'll have to give some thought to how the app could know which parent configs to trigger on when using more than the default, but i think it would be best to have a push approach when they do change. it could make sense to enable some sort of more-manual trigger, too. i'm open to discussing the balance further, but it does feel like a manual trigger is a little bit more of a work-around than a full solution, so i'm not sure that i would be comfortable suggesting that as the full solution.

travi avatar Sep 27 '21 20:09 travi

there are currently no events that we could listen for when settings are changed through the web ui to become out of sync with what is defined in the settings.yml

probot-settings is already doing this but only limiting its re-application of config to when the default_branch was changed https://github.com/probot/settings/blob/master/index.js#L35

The api route this is documented as being tied to does contain most of what the settings app can edit, labels notwithstanding. We would also likely need to listen to some sort of labels event to get this.

GitHub
Pull Requests for GitHub repository settings. Contribute to probot/settings development by creating an account on GitHub.

wraithgar avatar Sep 27 '21 21:09 wraithgar

probot-settings is already doing this but only limiting its re-application of config to when the default_branch was changed

you're right. clearly, some of these details have become a bit fuzzy and i'm not remembering the details clearly. however, it does still highlight my point. i would prefer to expand this approach of reacting to changes so that the app enforces the config as the source of truth. in addition to just re-syncing, i think opening a PR after re-syncing would provide better feedback about why the changes made through the ui disappeared. that also gives some guidance about how they could stick.

you're also correct that we would also need to listen for labels changes. in addition, we would need to listen for changes to each of the "plugin" topics. there is work to be done there, but we could do better.

we're probably getting a little of track, but i agree that these do all have an impact on this topic. the combination of all of this has been a big factor holding back implementation of this feature.

i do believe that perfection is the enemy of good. i'm open to discussing the balance further to get us moving forward. i just want to make sure that we do keep an eye on trade-offs and whether they are the right ones for moving toward better and keeping maintainability reasonable.

travi avatar Sep 27 '21 21:09 travi

Any progress on this @travi ? Let us know if there's anything the community can help out with, I'd be happy to be of assistance.

flugg avatar Apr 12 '22 15:04 flugg