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

[Mono.Security] Added Encode to AuthorityKeyIdentifierExtension and Subj... #1057

Merged
merged 2 commits into from
May 27, 2014
Merged

Conversation

lextm
Copy link
Contributor

@lextm lextm commented May 25, 2014

When trying to create new certificates via X509CertificateBuilder it is very important to have AuthorityKeyIdentifier and SubjectKeyIdentifier extensions configurable.

This pull request implements the required Encode methods of two classes and have been verified to be working.

I cannot build the .Tests project and that's why no unit test case is attached.

@monoadmin
Copy link

Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message.

@knocte
Copy link
Contributor

knocte commented May 25, 2014

An argument exception thrown in a method that receives no arguments? (That looks suspicious.)

Did you try building the unit tests via "make test" instead of MSBuild/xbuild?

@lextm
Copy link
Contributor Author

lextm commented May 25, 2014

ArgumentException was thrown from Decode, and I am not sure what should be used for Encode. Is InvalidOperationException better in this case?

I will build a Linux VM aside and see if make test helps.

@knocte
Copy link
Contributor

knocte commented May 25, 2014

Is InvalidOperationException better in this case?

The best thing to do is throw what MS.NET throws (even if it wrongly throws ArgumentException). That's why a unit test is very recommended to add, that checks this.

I will build a Linux VM aside and see if make test helps.

Fixing the .csproj to make it build in Windows is also welcomed.

@spouliot
Copy link
Contributor

Unit test are required. In this case you want to ensure that encode/decode can roundtrip with same data correctly.

Ideally you'll be able to run the existing one but that should not stop you from writing new tests.

@lextm
Copy link
Contributor Author

lextm commented May 26, 2014

@spouliot @knocte I just finished two simplest unit test files for each classes. What is the header I should use for the new source files?

What about the below

//
// SubjectKeyIdentifierExtensionTest.cs - NUnit Test Cases for 
//  Mono.Security.X509.Extensions.SubjectKeyIdentifierExtension
//
// Authors:
//  Lex Li  <support@lextm.com>
//
// Copyright (C) 2014 Lex Li
//

@spouliot
Copy link
Contributor

Please add the MIT license text to the header. Code is frequently copied between implementation and test files.

@lextm
Copy link
Contributor Author

lextm commented May 26, 2014

Done. Can you take a look?

spouliot added a commit that referenced this pull request May 27, 2014
[Mono.Security] Added Encode to AuthorityKeyIdentifierExtension and Subj...
@spouliot spouliot merged commit 99b5aad into mono:master May 27, 2014
@spouliot
Copy link
Contributor

Review and tested (locally). The .sources file for the Makefile is not updated (but I'll take care a it in a separate commit). Thanks!

@monoadmin
Copy link

Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message.

Red54 pushed a commit to Red54/mono that referenced this pull request Mar 15, 2019
…gc-suspend-loop

Move thread exit check within loop.
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.

4 participants