Runtime: [Question] Comments with <SecurityKernel ..> in corefx

[jamesqo]

There are a bunch of XML doc-like comments with <SecurityKernel ..> throughout the repo. See here: https://github.com/dotnet/corefx/search?utf8=%E2%9C%93&q=SecurityKernel&type=Code I stumbled across an unsafe method in SortedSet that was marked with it over here. Would it be OK to delete these?

[danmosemsft]

@karelz what do we currently recommend for code documentation? I don't see this covered in https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md.

[jkotas]

<SecurityKernel ..> comments are left overs from early version of security transparent/critical code annotations. They can be removed.

[tarekgh]

we need then to scan and delete all code under SecurityKernel

[WinCPP]

Hi,

This appears to be isolated piece of work and a pretty straightforward task of cleaning up? If my understanding is correct, can I take this up. Being new, it will help me get familiar with code base and processes.

[karelz]

Yes, this should be fairly straightforward. I sent you invite for collaborator, let me know when you accept and I can assign it to you ...

[WinCPP]

@karelz accepted the invite. Thank you.

[WinCPP]

I have done the changes, but I am not able to figure out from developer docs, what to do next i.e. file a review, etc.. Kindly advise.

Also I see that "outer loop" automated tests are failing on few platforms while they are passing on others. I am a bit confused as this was just removal of some code which was already commented... Trying to figure that out what is happening...

[karelz]

When you have changes (strong recommendation: in a branch), submit PR on GitHub.
Area owners will review. Don't merge master until the very end. Just react to feedback in your branch (pushes automatically update PR on GitHub).

Both inner and outer loop tests should pass in general. We ask contributors to run inner loop before submitting PR and relevant Outer loop subsets. In this case, I would skip local outer loop test run entirely - it is unlikely you are going to cause any problems, the more in outer loop.
Outer loop tests still contain some unreliable tests :( - we are working through them (e.g. Networking tests reliability is actively worked on).

BTW: I planned to put together simple newbie contributor guide to CoreFX over December - I didn't get beyond rough idea in OneNote :( ... I guess I should bump priority of that again.

[WinCPP]

@karelz Thanks for the direction, learnt the process while working on this issue. Local inner loop execution was successful. Have created a pull request and will wait for review feedback.

[karelz]

Great, thanks for helping out!

If you're interested in more contributions to CoreFX, I would recommend to grab some 'up for grabs' issues (which are not API proposals)
The most valuable/impactful things are:

1. 2.0.0 (next release) up for grabs issues
2. wishlist issues
3. simple issues for newbie contributors (not applied too much yet - we just started experimenting with the label recently)

[WinCPP]

@karelz thank you for helping out with the process and stuff, I have good idea about it now. I enjoyed contributing and will look into the above mentioned labels and grab something!

[jamesqo]

@WinCPP I see you have submitted a PR to fix this and it was merged. In the future, please put "Fixes #" in the description of the PR when submitting a patch that fixes an issue. For example, you could have put "Fixes dotnet/corefx#15374" in your PR description. That way, GitHub auto-closes that issue once the PR is merged.

[karelz]

Thanks for catching it @jamesqo!

[WinCPP]

@jamesqo Thank you, got it. Henceforth I'll do this way in the PR:

Title: "Fixes # - "
Description (comments): "Fixes #.

.

I thought just mentioning the issue number in title and description automatically links up.