Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update coding style and test facilities #477

Merged
merged 2 commits into from
Aug 12, 2024
Merged

update coding style and test facilities #477

merged 2 commits into from
Aug 12, 2024

Conversation

baentsch
Copy link
Member

@baentsch baentsch commented Aug 9, 2024

Addresses increasing problems with different clang-format versions (e.g., #473, #476); adopts mechanism from open-quantum-safe/liboqs#1861 (kudos to @SWilson4 ).

Anyone feeling strongly about the previous (more OpenSSL-oriented, but not totally so thoroughly tested) and the new (LLVM with OpenSSL's 4 indents), please speak up. Both options LGTM -- but the new variant seems to work more consistently across clang-format versions).

CI for coding style also already upgraded in preparation for a fix to #473 (and a general update to more current platforms).

@baentsch baentsch requested a review from feventura as a code owner August 9, 2024 13:15
@baentsch baentsch requested a review from a user August 9, 2024 13:15
@baentsch baentsch requested a review from bhess as a code owner August 9, 2024 13:15
scripts/do_code_format.sh Outdated Show resolved Hide resolved
Co-authored-by: Spencer Wilson <[email protected]>
Signed-off-by: Michael Baentsch <[email protected]>
@baentsch
Copy link
Member Author

baentsch commented Aug 9, 2024

@ryjones What rights do I need to look at and change PR merge rules to overcome the merge blocker in this PR? Two approvals incl. one from the person who did the original style-check code (@thb-sb ) are more than enough for this PR -- but I don't have permissions any more to do anything about this:
image

Who can grant these rights? Why do they need to be kept away from me? Behind this the by now "usual" reminder: Please consider answering the underlying question which rights a maintainer in LF needs (to get)? If answered, these problems would be resolved before they hit. It's really not funny (motivating, etc.) to keep banging into permissions limitations.

@hartm @dstebila fya as you initiated this. fwiw, I'll be mostly offline for the next week, but it'd be nice to find some answers when I'm back.

@SWilson4
Copy link
Member

SWilson4 commented Aug 9, 2024

It looks to me like it's due to "Require Review from Codeowners" (see Additional Setting under "Require a Pull Request Before Merging" here). Previously this would have been moot, but #458 added codeowners who now are required to approve things. Presumably the automation doesn't detect that the changes to, say, oqs_sig.c, which has @feventura as a codeowner, are trivial.

@ryjones
Copy link

ryjones commented Aug 9, 2024

OK I found the setting and turned it off. The issue was there were no CODEOWNERs set.

@ryjones
Copy link

ryjones commented Aug 9, 2024

This is a consequence of a change you made on 07 APR. The audit log shows:

Screenshot 2024-08-09 at 12 00 37

As @SWilson4 notes, the commit you made to add a CODEOWNERS resulted in your inability to merge.

@baentsch
Copy link
Member Author

This is a consequence of a change you made on 07 APR.

That I knew (and is obvious as I had the required GH permissions to do so at the time); the problem is that you removed my GH permissions (e.g., to update the rule set) since.

Hence, my question above was how can I keep maintaining the rule set (and also the project which I created)? Which GH permissions do I need? Or is that one of the things a LF maintainer should not be able to do?

@ryjones
Copy link

ryjones commented Aug 10, 2024

Looking into it I updated this PR

@baentsch baentsch merged commit 88aca9b into main Aug 12, 2024
47 checks passed
@ryjones ryjones deleted the mb-styletesting branch August 12, 2024 12:23
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.

3 participants