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

Expanding security assessment facilitator role definition. #815

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

IAXES
Copy link
Contributor

@IAXES IAXES commented Nov 2, 2021

  • Various edits to appease the mdl linter.
  • Enforcing some style changes (i.e. common indentation and
    trailing period literals in unordered lists, etc.).
  • Using a shell script to auto-generate/align the table of contents
    rendered at the top of the document.

Various edits to appease the `mdl` linter.

Enforcing some style changes (i.e. common indentation and trailing
period literals in unordered lists, etc.).
@IAXES
Copy link
Contributor Author

IAXES commented Nov 2, 2021

This was the minimum I could get the linter down to. We'd need a prescribed/pre-created mdl linter config file (i.e. that we pull and use locally) if we need 100% of the linter warnings/errors to go away.

governance/roles.md:11: MD007 Unordered list indentation
governance/roles.md:82: MD007 Unordered list indentation
governance/roles.md:248: MD013 Line length
governance/roles.md:251: MD013 Line length

Edit: all good now. Also, found the linter configuration.

@IAXES IAXES changed the title Expanding meeting facilitator role definition. Expanding security assessment facilitator role definition. Nov 2, 2021
Signed-off-by: Matthew Giassa <matthew@giassa.net>
… github.com:IAXES/tag-security into feature-add-sec-assessment-facilitator-role-details

Signed-off-by: Matthew Giassa <matthew@giassa.net>
@IAXES
Copy link
Contributor Author

IAXES commented Nov 2, 2021

Amended most recent commit to include signature.

Copy link
Collaborator

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

@IAXES this looks good. Left a few comments.

governance/roles.md Outdated Show resolved Hide resolved
governance/roles.md Outdated Show resolved Hide resolved
governance/roles.md Outdated Show resolved Hide resolved
governance/roles.md Outdated Show resolved Hide resolved
@IAXES
Copy link
Contributor Author

IAXES commented Nov 8, 2021

Updated as per feedback.

@ultrasaurus
Copy link
Member

I think it is very important not to repeat information that exists elsewhere, but rather link it. For example: soft/hard conflicts are presented here: https://github.com/cncf/tag-security/blob/main/assessments/guide/security-reviewer.md (I think that's a good place for them, though would be open to moving rather than copying if the community feels strongly that this information belongs in governance. If so, consider a small, specific "conflict of interest" doc that can be linked to from both places.)

@ultrasaurus
Copy link
Member

Note: this is very hard to review because of all the formatting changes included with (maybe?) substantive changes. Please make clear in the description what expansion of the role you are including, whether this is a proposed change in actual practice or an expanded narrative of what was originally written. This is critical for anything in /governance.

@IAXES
Copy link
Contributor Author

IAXES commented Nov 9, 2021

Note: this is very hard to review because of all the formatting changes included with (maybe?) substantive changes. Please make clear in the description what expansion of the role you are including, whether this is a proposed change in actual practice or an expanded narrative of what was originally written. This is critical for anything in /governance.

I'll do a 2-pass approach in the future (i.e. linter + cosmetic changes first round; substantive/major changes in a follow-up). The change is centered around the "Security review facilitator" / "Security Assessment Facilitator" section. The other changes through the document were just style updates and refactoring the table-of-contents at the top.

@IAXES
Copy link
Contributor Author

IAXES commented Nov 9, 2021

I think it is very important not to repeat information that exists elsewhere, but rather link it. For example: soft/hard conflicts are presented here: https://github.com/cncf/tag-security/blob/main/assessments/guide/security-reviewer.md (I think that's a good place for them, though would be open to moving rather than copying if the community feels strongly that this information belongs in governance. If so, consider a small, specific "conflict of interest" doc that can be linked to from both places.)

The existing location (i.e. security-reviewer) seems fine, IMHO. I just wanted to have a concrete quote/reference (in this case, copy) of it in the current document. If this was a rST/Sphinx doc, for example, I would've just put the example in a separate file and referenced it in both the governance and security-reviewer docs (i.e. made a macro out of the quote). No objections on my end if we should pull it from the governance doc to de-dupe content. :)

Copy link
Collaborator

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together @IAXES , couple requests for changes!

governance/roles.md Outdated Show resolved Hide resolved
governance/roles.md Outdated Show resolved Hide resolved
governance/roles.md Show resolved Hide resolved
governance/roles.md Outdated Show resolved Hide resolved
@lumjjb
Copy link
Collaborator

lumjjb commented Dec 1, 2021

Hi @IAXES just checking in if you've had some time to take a look at the comments!

@IAXES
Copy link
Contributor Author

IAXES commented Dec 4, 2021

Hi @IAXES just checking in if you've had some time to take a look at the comments!

Yep: reviewing now. :)

@stale stale bot added the inactive No activity on issue/PR label Feb 20, 2022
@stale stale bot removed the inactive No activity on issue/PR label Jun 15, 2023
@netlify
Copy link

netlify bot commented Jun 15, 2023

Deploy Preview for tag-security ready!

Name Link
🔨 Latest commit 153e5ae
🔍 Latest deploy log https://app.netlify.com/sites/tag-security/deploys/6553b9e6976bcf0008ae58fc
😎 Deploy Preview https://deploy-preview-815--tag-security.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@anvega anvega requested a review from lumjjb June 15, 2023 22:50
Changes to sentence case in toc
Reworded most responsible to primary individual responsible in making progress in the assessment process with support of reviewers
Copy link
Collaborator

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

just a minor nit on practicality of permission assignment in github, else LGTM

of at least two Chairs).
4. The issue is updated with the assignment of the project lead as "assignee"
alongside the TAG Leadership member.
5. Project Leads will be given the OWNER role of the directory or sub-directory
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this technically possible? I don't believe you can do this in github today? Someone can clarify? @PushkarJ had a bunch of cool github gizmos he implemented through the bot though, so maybe that's possible?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's possible. This was a hindrance when I was the assessed rather than the assessor. We can accomplish so today by declaring ownership of directories or files in CODEOWNERS: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners#example-of-a-codeowners-file

@achetal01
Copy link
Collaborator

I m good with the changes...Thank You IAXES for all the hard work in updating the document.

@anvega
Copy link
Collaborator

anvega commented Sep 12, 2023

Nudge @sublimino

@anvega anvega requested a review from mnm678 November 14, 2023 03:33
Copy link
Collaborator

@JustinCappos JustinCappos left a comment

Choose a reason for hiding this comment

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

This looks fine to me.

governance/roles.md Outdated Show resolved Hide resolved
@sublimino
Copy link
Member

One typo fix suggested, then LGTM too

Copy link
Collaborator

@mnm678 mnm678 left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: Andrew Martin <sublimino@gmail.com>
Signed-off-by: Justin Cappos <justincappos@gmail.com>
Copy link

stale bot commented Mar 17, 2024

This issue has been automatically marked as inactive because it has not had recent activity.

@stale stale bot added the inactive No activity on issue/PR label Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive No activity on issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants