Code owners attempt 2


#1

Hello all,

A while ago I proposed code owners for the packaging files. We tried this[1] but didn't work in our setup. It turns out that those mentioned as owners need permissions on the repository. In our case the packaging team (not individual members) has no permission.

There are 3 options:

1) Remove the file since it doesn't work
2) Create a packaging-core group with the intersection of both groups
3) Give @theforeman/packaging permissions on core

Personally I'd really like us to work together better and think this could reduce our packaging issues but have no preference between 2 or 3.

[1]: https://github.com/theforeman/foreman/blob/develop/.github/CODEOWNERS


#2

This could use some opinions. Right now we’re missing a notification system for packagers that these changed. A 4th option is to build some sort of notification system ourselves but I’d like to avoid that.


#3

Here, let me create a poll for you:

  • Remove the file since it doesn’t work
  • Create a packaging-core group with the intersection of both groups
  • Give @theforeman/packaging permissions on core

0 voters


#4

Ultimately I trust our packagers, since they control what goes to the users - and in any case, we can revert changes if someone does something bad, it’s all in the git history :slight_smile:, so (3) it is!


#5

I think another option is to use https://about.pullapprove.com, free for open source projects. I think it can do what we are after and a little more.


#6

To give a little more context to the above…

You can set specific rules for changes to certain files, see the example on https://docs.pullapprove.com/.

disclaimer :wink: I’ve never actually used this, but it looks like the kind of thing we are after.


#7

I’ll put in the obilgatory comment that this is a paid-for commercial service, with a free tier, so factor in considerations of what happens if they decide to remove the free tier later. Also note their terms of service explicitly permit sharing data with third-party organisations. I’m not a fan of that :slight_smile:


#8

This has been open for a while. Should we close this and move forward based on the result?


#9

Ping? If the current situation continues we might as well drop our nightlies since they’re broken and unusable anyway. There needs to be a dialogue between developers and packaging. Code owners may not not be the ideal or even correct solution, but I don’t hear any other suggestions.


#10

@ekohl the poll is conclusive, and no objections have been raised. Let’s move forwards. I’ve added write access to theforeman/foreman to the packaging team on GitHub - is there anything else you need?


#11

Thanks! Now we just need some time to try it out and evaluate the result.


#12

To clarify the result of this discussion, does this inherently give maintainer access to theforeman/foreman repository for all current and future packaging team members? I ask for two reasons:

  1. This provides a loop-hole around the agreed upon process for becoming a maintainer and having access to the commit bit and potentially provides access to users who may not have had this before
  2. Any future additions to the packaging team may have to come with further scrutiny since a by product would be granting core commit bit

I raise this for both understanding, vigilance and to ensure that everyone understands the full scope of this change.


#13

Thanks for the warning. In my opinion core should trust the packaging team to do the right thing on the release branches.

If someone in packaging does not have commit access already because they’ve been an usual committer to core, I doubt they would merge anything in core, especially when not related to packaging issues (e.g - Gemfile changes, assets pipeline changes).

Generally I agree about point 2, but in any case my opinion is that much is to be gained by trusting ‘packagers’ to do TheRightThing*tm in core (mostly restricted to what I said in the previous paragraph) and a lot of time is to be lost by putting more barriers to that.


#14

I agree with @dLobatog, and I’d also expect the reverse - that existing maintainers would notice if a packager goes too far, and take action. That could be a simple discussion about why a commit happened, all the way up to reverting the commit if needed. Git is a version control system, after all, we have all the history if we need it :wink:


#15

To be honest, I think we’re taking this the wrong way. I think, commit access to foreman core should be justified by helping to maintain the codebase. I don’t want to have a loop-hole. I can only speak for myself, but the way I see it is the following: I have commit access to core, but don’t have commit access to packaging. And I think, that’s totally fine. I haven’t earned that right in foreman-packaging. This is not an issue of not trusting people (I do) or not needing help (we do), but contributors should get a proper review and people need to know the codebase and product well to make a proper review. I think commit permissions should be earned.

When releasing a plugin, one of my release todos is to send PRs to foreman-packaging to update the packages. Can’t we agree, that we only merge core PRs that have a corresponding packaging PR when needed? Ideally, a contributor would file the PR with the core PR. If that’s too much to ask, a maintainer can either help and file the PR or as someone from the packaging team for help. We don’t need any special permissions or loop-holes permissions for that.

Just my 2 cents. I hope, we find an easy solution for our issue. That is broken nightlies.


#16

Thanks for bringing this up. It’s time for a short retrospective.

I had hoped that it would loop us into conversations that changed packaging files. It appears there’s no such granularity and you either follow the entire repository or none of it (modulo those PR’s you’re participating in).

There are also quite a few PR’s being merged that don’t have corresponding packaging changes, or only partially. For example https://github.com/theforeman/foreman/pull/5096 did have the dependencies packaged but there wasn’t a PR to change the package itself. If time had been invested there https://github.com/theforeman/foreman/pull/5275 was probably caught during the review phase.

Other examples can be found in the node packaging.

Overall I’m not happy with the result of the experiment because it didn’t achieve the goal I had hoped for: more cooperation between core devs and packaging.

I’d be interested in what others think, but I’m leaning to reverting the change and try to find a real solution.


#17

My gut feeling is the that the question we need to answer is “How does a core developer know if something needs packaging changes?”. I believe we all want to do this better, so this isn’t a question of getting better communication, but rather how to know if such changes impact packaging.

Asking all core devs to understanding the packaging probably isn’t realistic - likewise, asking a packaging member to review every core PR isn’t either. Does this come down to more automated tests? Should we be building scratch packages for core PRs? (I know that will take a lot more Jenkins time, but I’m still trying to get us more slaves :stuck_out_tongue:)