Git Repo Admins

[scottrigby]

Git Repo Admins

[torstenwalter]

If you need an a helping hand than I could support as repository admin. From my understanding repository admins should not approve PRs for charts unless they are maintainer of the chart or one of the maintainers of a chart explicitly asks for it.

@scottrigby You did a great job in setting all this up and driving discussions. At least I would appreciate if you could continue helping as admin.

[timm088]

Cool, thanks for the explanation @scottrigby , all makes sense now

[scottrigby]

Hey @timm088 👋 thanks for asking! This issue needs clarification and closure.

There was a little confusion above - the CODEOWNERS file already exists, and added your ownership for the consul exporter chart:

helm-charts/.github/CODEOWNERS

Line 13 in c30ca33

/charts/prometheus-consul-exporter @timm088

That means you should already get a review request whenever someone opens a PR with any changes on it 👍 there should be nothing for there to do there.

@torstenwalter 's suggestion above was for all the chats in this repo to have at least two maintainers (there's another issue for that: #21). @gkarthiks made a PR to add him as a co-maintainer of your chart in #50. It's up to you though as a (currently only) maintainer of that chart to approve his request in that PR or not.

This issue was me asking all the current prom chart maintainers opinion on how repo administration should work (if people would like to rotate, vote on people who have shown they want to and are able to do it, etc). @torstenwalter volunteered earlier, and we worked together to set up a few other community chart repos (grafana and jenkins), so I added him as a repo admin. We now have a PROCESSES doc file for this repo, so you can all make PRs against that and discuss any additional process ideas and needs you have there.

For now I think this issue is done because it's not just me as admin anymore, so I'm going to close it. Let's all continue in the other issues 🙂

[monotek]

I guess everyone of the chart maintainers can also review non app specific stuff such as image tag updates and so on, so the abiltiy to "merge PRs for other charts they don't maintain" makes sense to me as this would fasten up review processes.

[torstenwalter]

@torstenwalter Also agree charts with a single maintainer should ideally have at least two. Hopefully people can work that out in #21. Do you want to move that part of your comment there?

Just moved it there #21 (comment)

[torstenwalter]

I just opened PR #44 which adds CODEOWNERS according to the list above.
@scottrigby I saw that you defined them in #1. This splits that up so that we could merge it independent of that.

I also adjusted the syntax to /chart/<name-of-chart> @maintainer.

It matches any files in the chart directory at the root of the repository and any of its subdirectories.
Without the leading / it would also match directories found somewhere else. It's unlikely that those names would be used, but it does not harm to do it this way.

Also added:

## changes to CODEOWNERS should be reviewed by repository admins
/CODEOWNERS @helm-charts-admins

The intention is that changes to the CODEOWNERS file also needs to be reviewed by one of the repository admins. One could argue, that it could delay the process of adding maintainers. In case one maintainer of a chart is added to another chart that is true! In case an external person is added that's not really the case as one of the repository administrators would need to grant him/her write permissions to the repository anyhow.

I also added a draft for PROCESSES.md, which at the moment just outlines how to add maintainers. We could use that file for further process documentation if needed.

When introducing CODEOWNERS we will have an issue with charts which only have one maintainers. If that maintainer make a change then he is not able to approve it. Repository admins would need to use their "superpower" to override the required review and can merge it. That might be ok for the start, but I would suggest that we try to add at least a second maintainer there.

Affected charts are:

• charts/prometheus-consul-exporter @timm088
• charts/prometheus-couchdb-exporter @gkarthiks
• charts/prometheus-mongodb-exporter @steven-sheehy
• charts/prometheus-postgres-exporter @gianrubio
• charts/prometheus-rabbitmq-exporter @Juanchimienti
• charts/prometheus-redis-exporter @acondrat
• charts/prometheus-snmp-exporter @Miouge1
• charts/prometheus-to-sd @acondrat

Would be great if we could fined volunteers here or even better if the chart maintainers could try to motivate people who already contributed to the chart to become maintainer.

[scottrigby]

@torstenwalter I updated your #44 PR title to be just about the PROCESSES documentation (which is great!), because /.github/CODEOWNERS already exists (I added it there in #12, for tidiness).

I also made a review comment to include some of the processes in the chat above, like what you just mentioned about admin and chart maintainer roles relating to PRs.

[scottrigby]

@torstenwalter Also agree charts with a single maintainer should ideally have at least two. Hopefully people can work that out in #21. Do you want to move that part of your comment there?

[scottrigby]

@torstenwalter you’re now a member of the @prometheus-community/helm-charts-admins team. So we have at least a few people to help with this side of things for now.

Let's all make sure processes like team membership (criteria, responsibilities and limitations) explicit in the PROCESSES doc being added in #44.

Once that's merged, we can probably close this issue and make further amendments to the PROCESSES doc in follow-up PRs, and discuss other questions or ideas that come up in new issues.

[timm088]

@scottrigby @torstenwalter not entirely sure whats being requested?
Haven't modified the consul-exporter chart in a while, but would be happy to assist where it makes sense.

Is the request here to try to gather support from existing contributors on that chart, to become CODEOWNERs?

Or , nothing to do and just an FYI?