tinkerbell / hook

In-memory Operating System Installation Environment for Executing Tinkerbell Workflows
Apache License 2.0
102 stars 49 forks source link

Few fixes from after #105 #130

Closed mmlb closed 2 years ago

mmlb commented 2 years ago

Description

Some fixes from after #105 was merged.

Why is this needed

Fixes things broken by refactor.

How Has This Been Tested?

Manual tests.

mmlb commented 2 years ago

Hey @ScottGarman @jacobweinstock can y'all take another look please?

jacobweinstock commented 2 years ago

Looking and FYI this will need @thebsdbox or @tstromberg in order to get merged.

mmlb commented 2 years ago

@jacobweinstock you're still pending as "requested changes" afaik all have been addressed. Is there anything else?

mmlb commented 2 years ago

Looking and FYI this will need @thebsdbox or @tstromberg in order to get merged.

Haven't seen much activity from either in this repo in a while, an exception may be in order. I've also opened up #131 to add myself as a maintainer.

jacobweinstock commented 2 years ago

Looking and FYI this will need @thebsdbox or @tstromberg in order to get merged.

Haven't seen much activity from either in this repo in a while, an exception may be in order. I've also opened up #131 to add myself as a maintainer.

sure, you already have approver permissions but don't know how moving to maintainer helps get PR's from you merged though. I think another approver would be a more idea option over an exception/override from a maintainer.

mmlb commented 2 years ago

Looking and FYI this will need @thebsdbox or @tstromberg in order to get merged.

Haven't seen much activity from either in this repo in a while, an exception may be in order. I've also opened up #131 to add myself as a maintainer.

sure, you already have approver permissions but don't know how moving to maintainer helps get PR's from you merged though. I think another approver would be a more idea option over an exception/override from a maintainer.

Yeah. I figure we always want someone "above the ladder" to approve. But then what do we do about maintainers? IDK if maintainers need approval from other maintainers or if we go "one rung down the ladder" for those, or both.

mmlb commented 2 years ago

I haven't seen much interaction from @thebsdbox on this repo, ditto @tstromberg who coincidentally has a single commit for auxillary code (lint-install setup). idk if makes sense to use that commit as the basis for approver status. This repo all-in-all is in need of more active people in the roles, or loosening of gates.

jacobweinstock commented 2 years ago

I haven't seen much interaction from @thebsdbox on this repo, ditto @tstromberg who coincidentally has a single commit for auxillary code (lint-install setup). idk if makes sense to use that commit as the basis for approver status. This repo all-in-all is in need of more active people in the roles, or loosening of gates.

needing more active folks is definitely what we should be focusing on. loosening gates doesn't really make sense. As you have noted Thomas only has a single commit and yet he is still an approver.

Also, the current governance handles approving PRs the same for all people who open a PR. It doesnt matter what the role of that person is. The process is the same.

mmlb commented 2 years ago

I haven't seen much interaction from @thebsdbox on this repo, ditto @tstromberg who coincidentally has a single commit for auxillary code (lint-install setup). idk if makes sense to use that commit as the basis for approver status. This repo all-in-all is in need of more active people in the roles, or loosening of gates.

needing more active folks is definitely what we should be focusing on. loosening gates doesn't really make sense. As you have noted Thomas only has a single commit and yet he is still an approver.

Also, the current governance handles approving PRs the same for all people who open a PR. It doesnt matter what the role of that person is. The process is the same.

Hmm I guess I was confused about the approver role, for some reason I thought another maintainer would be wanted to approve of a mainter's code. That not being the case makes a ton of sense. At this point though, this repo has just one active member that can approve PRs, me. Let me put out a call for approvers in slack.