Skip to content

Comments

Apply OTC changes to gitaddrev#50

Closed
romen wants to merge 1 commit intoopenssl:masterfrom
romen:addrev_otc
Closed

Apply OTC changes to gitaddrev#50
romen wants to merge 1 commit intoopenssl:masterfrom
romen:addrev_otc

Conversation

@romen
Copy link
Member

@romen romen commented Jan 5, 2020

After the changes in openssl/web#146
gitaddrev (on which addrev depends) required an update to count OTC
approvals rather than OMC approvals.

After the changes in openssl/web#146
`gitaddrev` (on which `addrev` depends) required an update to count OTC
approvals rather than OMC approvals.
@romen romen requested a review from levitte January 5, 2020 08:55
@romen
Copy link
Member Author

romen commented Jan 5, 2020

My Perl-foo Perl-fu is as usual very close to zero, so please have an in-depth look to see if what I did makes sense, as I don't fully grasp all the details of how things are checked!

edit: typo

Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I haven't tested, but I see no reason why this should not work.

@levitte
Copy link
Member

levitte commented Jan 5, 2020

Side note: -fu, not -foo.

https://en.wiktionary.org/wiki/-fu

mattcaswell
mattcaswell previously approved these changes Jan 6, 2020
@mattcaswell mattcaswell dismissed their stale review January 6, 2020 11:37

I thought of a question

@mattcaswell
Copy link
Member

How does this impact the review headers for other repos? So, with addrev you can specify --web or --tools to specify that you are applying headers for a different repo than the main code one.

For that matter what are the review rules for other repos. For the code repo it is clear. But what about web and tools? It kind of makes sense at least for tools to be OTC owned?

@romen
Copy link
Member Author

romen commented Jan 6, 2020

How does this impact the review headers for other repos? So, with addrev you can specify --web or --tools to specify that you are applying headers for a different repo than the main code one.

For that matter what are the review rules for other repos. For the code repo it is clear. But what about web and tools? It kind of makes sense at least for tools to be OTC owned?

I believe OMC should clearly state this officially somewhere to clear the ambiguity: personally I would say that web should not fall under OTC control as I believe the fundamental role of the website goes beyond the area of interest of the OTC and more under the direct functions of the OMC.

tools seems to include mostly stuff that appears pretty much of OTC interest, but maybe at least tools/QueryApp should be exempt from OTC control?

That's of course just my personal opinion, and I only use a tiny fraction of the tools in tools!

That being said, until your comment I did not even think that addrev was used also outside of reviews for the openssl/openssl repo, so maybe @levitte could suggest something better for handling repos under OTC or under OMC control?

Anyway, until the OMC states otherwise, this PR must be merged by an OMC member.

Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did similiar changes before I noticed your PR.
I left the [OMC] tag in so that it printed both [OMC] [OTC] in the --list part.
Its the same otherwise..

@paulidale
Copy link
Contributor

@mattcaswell didn't we discuss some of the ownership at the F2F? I've a recollection that at least one of the repos looked like it should be jointly owned (i.e. needed splitting).

Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want the wider issues brought up here stopping progress on reviews. So approving this for now.

openssl-machine pushed a commit that referenced this pull request Jan 13, 2020
After the changes in openssl/web#146
`gitaddrev` (on which `addrev` depends) required an update to count OTC
approvals rather than OMC approvals.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Matt Caswell <[email protected]>
(Merged from #50)
@mattcaswell
Copy link
Member

Pushed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants