This pull request has merge conflicts that must be resolved before it can be
merged. @cdoern please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

Add hold waiting on design doc

Removed hold as design doc was merged.

https://github.com/Mergifyio rebase

rebase

☑️ Nothing to do

This pull request has merge conflicts that must be resolved before it can be
merged. @cdoern please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

thanks for catching the prettytable dependency @russellb , this should be all set now

@leseb @markstur If your changes requested are still relevant can you point them out or if not remove the changes requested. It's the only thing standing in the way of merging the PR.

I tried to apply hold to catch the merge before it went in but just missed it.

I talked with @cdoern about the merge process and how we have Mergify configured to explicitly NOT merge when someone has previously requested changes and not come back and explicitly ACKed that they have been resolved. My preference would have been to ping both @markstur and @leseb to give them a chance to give that ACK. Only if they didn't respond would I consider dismissing the review.

I know @cdoern had all the best of intentions here and genuinely felt that it was acceptable to dismiss the reviews if the feedback was addressed. In his defense, this is not documented anywhere! As I think about it, that's probably fine when the feedback in the review was simple enough. @leseb at least was doing more in depth review and has been very responsive on this.

@cdoern has offered to revert the PR to give a chance for the final review responses. I told him I did not feel it was worth the churn in this case.

@markstur @leseb would you both be OK taking another look at this PR (post-merge) to see if there was anything else you'd like to see addressed? If it's something not captured in a follow-up issue, please raise it. If you have any more significant concerns, we can consider reverting whatever portion is affected, but I'd rather hold off on that churn unless it's justified.

Hopefully that makes sense to everyone! Let me know if I can help discuss anything further.

I tried to apply hold to catch the merge before it went in but just missed it.

I talked with @cdoern about the merge process and how we have Mergify configured to explicitly NOT merge when someone has previously requested changes and not come back and explicitly ACKed that they have been resolved. My preference would have been to ping both @markstur and @leseb to give them a chance to give that ACK. Only if they didn't respond would I consider dismissing the review.

I know @cdoern had all the best of intentions here and genuinely felt that it was acceptable to dismiss the reviews if the feedback was addressed. In his defense, this is not documented anywhere! As I think about it, that's probably fine when the feedback in the review was simple enough. @leseb at least was doing more in depth review and has been very responsive on this.

@cdoern has offered to revert the PR to give a chance for the final review responses. I told him I did not feel it was worth the churn in this case.

@markstur @leseb would you both be OK taking another look at this PR (post-merge) to see if there was anything else you'd like to see addressed? If it's something not captured in a follow-up issue, please raise it. If you have any more significant concerns, we can consider reverting whatever portion is affected, but I'd rather hold off on that churn unless it's justified.

Hopefully that makes sense to everyone! Let me know if I can help discuss anything further.

yeah I was trying to ACK those when it merged. Now I'm looking at main. So far the concerns seem to be fixed or no longer relevant in what was merged

@markstur @leseb I am really sorry for not letting you guys ACK the PR before merging it, it was an honest mistake as I genuinely thought stale reviews which I had either made follow up issues for or addressed in the PR, could be dismissed. I did not realize this was not a normal practice and will be careful doing it in the future. Please feel free to be honest with me and let me know if you'd like this reverted, I really apologize.

@markstur @leseb I am really sorry for not letting you guys ACK the PR before merging it, it was an honest mistake as I genuinely thought stale reviews which I had either made follow up issues for or addressed in the PR, could be dismissed. I did not realize this was not a normal practice and will be careful doing it in the future. Please feel free to be honest with me and let me know if you'd like this reverted, I really apologize.

@cdoern No worries. No revert needed for any of mine. Just double-checking to make sure they are resolved or no longer relevant in main. So far, so good.

@markstur @leseb I am really sorry for not letting you guys ACK the PR before merging it, it was an honest mistake as I genuinely thought stale reviews which I had either made follow up issues for or addressed in the PR, could be dismissed. I did not realize this was not a normal practice and will be careful doing it in the future. Please feel free to be honest with me and let me know if you'd like this reverted, I really apologize.

@cdoern No worries, I'm not offended by how things have unfolded. There's no need to revert; we have follow-ups in place, and any missed items will be addressed.