Conversation
| :tags: Architecture | ||
| :implemented: YES | ||
| :version: 1 | ||
| :parent_covered: YES |
There was a problem hiding this comment.
YES: Together with tool_req__docs_req_link_security_to_arch``
|
Documentation preview for this pull request is available at: |
Implement the tooling for gd_req__arch_linkage_requirement_security and gd_req__req_linkage_security (process_description eclipse-score#796): - tool_req__docs_req_link_security_to_arch: security relevant requirements may only be satisfied by security relevant architecture elements - tool_req__docs_arch_link_nonsec_to_sec_req: non-security architecture elements may not fulfil security relevant requirements or AoUs - tool_req__docs_req_link_security_child: a security relevant requirement with children keeps at least one security relevant child Graph checks support an optional `new_check: true` key, which reports findings as infos until existing data has been cleaned up. The two new graph checks start in this mode. tool_req__docs_arch_link_security now also covers `comp` and is set to PARTIAL, as only the `implements` link is checked. File based rst tests now also match infos of new checks, which enables the previously inverted test in test_invalid_graph.rst. process_description is pinned to 5f80e45 until eclipse-score#796 is released. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
e5ad8e3 to
6a6d466
Compare
MaximilianSoerenPollak
left a comment
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
Some questions need answers first.
I thought a bit about it and think we need to figure this out first before we can go along.
Though the overall tests (graph_check) in the metamodel etc. makes all sense 💯
| .. tool_req:: Security: Security requirement keeps a security child | ||
| :id: tool_req__docs_req_link_security_child | ||
| :tags: Architecture | ||
| :implemented: YES | ||
| :version: 1 | ||
| :parent_covered: YES | ||
| :satisfies: gd_req__req_linkage_security[version==1] | ||
|
|
||
| Docs-as-Code shall enforce that every security relevant requirement (Security == YES) which | ||
| has child requirements (``derived_from``) has at least one security relevant child requirement | ||
| (Security == YES). | ||
|
|
||
| .. note:: | ||
| Parent-child pairs across repository boundaries are not checked, because either the | ||
| children are missing in the build or the parent is external. |
There was a problem hiding this comment.
The process seems not complete here, as I don't see this in the process.
Is this something that will be coming in after or where the wording will change?
Cause the linked requirement doesn't specify this part.
There was a problem hiding this comment.
Usually we check all links, regardless of cross repo or not. Not checking when cross repo would actually require some effort.
| "Explanations are mandatory for graph checks." | ||
| ) | ||
| # New checks only report infos until existing data has been cleaned up. | ||
| is_new_check = bool(check_config.get("new_check", False)) |
There was a problem hiding this comment.
This is a good feature. I think we can add this in this PR as an introduction even if it should be a seperate one.
There was a problem hiding this comment.
I'm not sure... our current info messages are ignored by everyone. And we will not see these in our downstream tests?
| if invalid_needs: | ||
| msg = f"is valid but links to invalid need(s): {invalid_needs}" | ||
| log.warning_for_need(need, msg, is_new_check=True) | ||
|
|
There was a problem hiding this comment.
I think it would be better to add a new item to the graph checks.
Similar to the new_check we should add a thing that tells it if ALL have to fulfilled or if just one has to be.
Then we can adapt the logic of the current code a bit and don't have to add a single new check that does it for this circumstance?
@AlexanderLanin @a-zw thoughts on this?
There was a problem hiding this comment.
Adding that mechanism to the metamodel.yaml is good.
I'm not convinced about the name "new_check" because "new" is vague and relative. Maybe "info_only"?
There was a problem hiding this comment.
It's consitent with internal naming there we also have it called new_check but yes it probably should be like info_only or similar.
| :status: valid | ||
| :mitigated_by: feat_req__parent__QM_invalid | ||
| :expect_not: invalid need(s) | ||
| :expect: is valid but links to invalid need(s): {'feat_req__parent__QM_invalid'} |
There was a problem hiding this comment.
This test will not be green.
The warning is active but only as a new_check meaning it's an info.
The test only chekcs for warnings.
That's why it was testing the inverse earlier, which is the only thing we can do until we have the valid check fully active.
| # Some warnings are suppressed in conf.py, so the set here is already limited. | ||
| warnings = [strip_ansi_codes(w) for w in app.warning.getvalue().splitlines()] | ||
| # New checks report infos instead of warnings, they are expected in the same way. | ||
| warnings += [strip_ansi_codes(w) for w in app.status.getvalue().splitlines()] |
There was a problem hiding this comment.
This for sure does not belong in this PR.
This might be indeed the case that we want to test this but this needs a discussion first.
4 participants
📌 Description
The PR implements new process requirements eclipse-score/process_description#796
🚨 Impact Analysis
✅ Checklist