Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions docs/internals/requirements/requirements.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1085,19 +1085,21 @@ is described in the platform.
.. tool_req:: Decision Record Type
:id: tool_req__docs_dec_rec_type
:tags: Process / Other
:version: 1
:version: 2
:implemented: YES
:satisfies:
gd_req__process_management_build_blocks_attr[version==1],
gd_req__process_management_build_blocks_link[version==1],

Docs-as-Code shall support the Decision Record (dec_rec) need type with the following options:
Docs-as-Code shall support the Decision Record (dec_rec) need type with the following mandatory options:

* ``id`` (mandatory, pattern: ``dec_rec__<category>__<slug>``; category and slug contain letters, numbers, underscores, or hyphens)
* ``status`` (mandatory, one of: proposed, accepted, deprecated, rejected, superseded)
* ``context`` (mandatory)
* ``decision`` (mandatory)
* ``consequences`` (optional)
* ``tracking`` (optional, link to a GitHub issue)
* ``tracking`` (mandatory, link to a GitHub issue or pull request in the ``eclipse-score`` organization)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

tracking for forks is in other organizations

* ``version`` (mandatory)
* ``consequences`` (mandatory, non-empty)

A Decision Record shall be able to link to other needs via the optional ``affects`` link.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
A Decision Record shall be able to link to other needs via the optional ``affects`` link.
A Decision Record shall be able to link to requirements via the optional ``affects`` link.

The PR description says "Constrain 'affects' link to requirement types only". If that is true, then this requirement should be adapted accordingly.

Likewise the implementation in metamodel.yaml. Currently, it allows "ANY" and not just requirements.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@a-zw The other need is by intention, because a decision could also influence the architecture and other stuff. Not only requirements. Therefore this is not recommended to change it in this way.


Expand Down
6 changes: 5 additions & 1 deletion src/extensions/score_metamodel/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@

logger = logging.get_logger(__name__)
env = Environment()
GITHUB_TRACKING_LINK_REGEX = (
r"(?P<url>https://github\.com/eclipse-score/"
r"(?P<repo>[^/]+)/(?:issues|pull)/(?P<number>\d+))"
)
Comment on lines +42 to +45

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
GITHUB_TRACKING_LINK_REGEX = (
r"(?P<url>https://github\.com/eclipse-score/"
r"(?P<repo>[^/]+)/(?:issues|pull)/(?P<number>\d+))"
)
GITHUB_TRACKING_LINK_REGEX = (
r"(?P<url>https://github\.com/[^/]+/"
r"(?P<repo>[^/]+)/(?:issues|pull)/(?P<number>\d+))"
)

Keep it as relaxed as before. Checking the URL for validity happens via the regex in metamodel.yaml and we must not be more restrictive here.

Currently, the problem is that this affects mitigation_issue and probably not intentionally.


local_check_function = Callable[[Sphinx, NeedItem, CheckLogger], None]
graph_check_function = Callable[[Sphinx, NeedsView, CheckLogger], None]
Expand Down Expand Up @@ -282,7 +286,7 @@ def setup(app: Sphinx) -> dict[str, str | bool]:
app.config.needs_string_links.setdefault(
"github_issue_linker",
{
"regex": r"(?P<url>https://github\.com/[^/]+/(?P<repo>[^/]+)/issues/(?P<number>\d+))",
"regex": GITHUB_TRACKING_LINK_REGEX,
"link_url": "{{url}}",
"link_name": "{{repo}}#{{number}}",
"options": github_issue_options,
Expand Down
11 changes: 6 additions & 5 deletions src/extensions/score_metamodel/metamodel.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1051,14 +1051,15 @@ needs_types:
dec_rec:
title: Decision Record
mandatory_options:
id: ^dec_rec__[0-9a-zA-Z_-]+__[0-9a-zA-Z_-]+$

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

id regex is not required

status: ^(proposed|accepted|deprecated|rejected|superseded)$
context: ^.+$
decision: ^.+$
optional_options:
consequences: ^.*$
tracking: ^https://github\.com/[^/]+/[^/]+/issues/\d+$
context: ^[\s\S]+$
decision: ^[\s\S]+$
tracking: ^https://github\.com/eclipse-score/[^/]+/(?:issues|pull)/\d+(?:#[A-Za-z0-9_-]+)?$
consequences: ^[\s\S]+$
Comment on lines +1058 to +1059

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

new mandatory attributes can only be added on next major release

optional_links:
affects: ANY
parts: 3

# Extra link types, which shall be available and allow need types to be linked to each other.
# We use a dedicated linked type for each type of a connection, for instance from
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,72 @@

.. test_metadata:: Test Decision Record Type
:id: test_metadata__dec_rec
:partially_verifies_list: tool_req__docs_dec_rec_type
:fully_verifies_list: tool_req__docs_dec_rec_type[version==2]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also verify that a wrong id pattern results in a warning.

:test_type: requirements_based
:derivation_technique: requirements_based

Tests that the dec_rec need type with its mandatory attributes and the
affects link is supported.
Tests that the dec_rec need type supports its mandatory attributes and affects
link, and that its tracking option accepts GitHub issue and pull request links.


.. dec_rec:: Test Decision Record 1
:id: dec_rec__test__decision_record_1
:id: dec_rec__platform__feature_delivery
:status: accepted
:context: A decision record is needed for the test.
:decision: Use a dedicated decision record need type.
:affects: dec_rec__test__decision_record_2
:tracking: https://github.com/eclipse-score/docs-as-code/issues/1
:version: 1
:consequences: Provides traceability for decisions.
:affects: dec_rec__feature__follow_up


.. dec_rec:: Test Decision Record 2
:id: dec_rec__test__decision_record_2
:id: dec_rec__feature__follow_up
:status: proposed
:context: This decision record is affected by another decision.
:decision: Keep the relationship explicit.
:tracking: https://github.com/eclipse-score/score/pull/2#discussion_r3
:expect_not: dec_rec__feature__follow_up.tracking (https://github.com/eclipse-score/score/pull/2#discussion_r3): does not follow pattern
:version: 1
:consequences: Allows linking between related decisions.


.. dec_rec:: Decision Record with missing tracking
:id: dec_rec__logging__no_tracking
:status: accepted
:context: The tracking link is missing.
:decision: Require tracking for every decision record.
:version: 1
:consequences: Decisions must refer to an issue or pull request.
:expect: dec_rec__logging__no_tracking: is missing required attribute: `tracking`.


.. dec_rec:: Decision Record with missing consequences
:id: dec_rec__logging__no_consequences
:status: accepted
:context: The consequences are missing.
:decision: Require consequences for every decision record.
:tracking: https://github.com/eclipse-score/score/pull/2
:version: 1
:expect: dec_rec__logging__no_consequences: is missing required attribute: `consequences`.


.. dec_rec:: Decision Record with invalid tracking organization
:id: dec_rec__dac__invalid_tracking_org
:status: accepted
:context: Tracking links must belong to the eclipse-score organization.
:decision: Reject links owned by other organizations.
:tracking: https://github.com/other-org/score/pull/3
:version: 1
:consequences: Tracking is restricted to the eclipse-score organization.
:expect: dec_rec__dac__invalid_tracking_org.tracking (https://github.com/other-org/score/pull/3): does not follow pattern


.. dec_rec:: Decision Record with missing status
:id: dec_rec__proc__missing_status
:context: The status is missing.
:decision: Required fields remain enforced.
:tracking: https://github.com/eclipse-score/score/issues/1
:version: 1
:consequences: Every decision record must declare its status.
:expect: dec_rec__proc__missing_status: is missing required attribute: `status`.
53 changes: 52 additions & 1 deletion src/extensions/score_metamodel/tests/test_metamodel_load.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,67 @@
#
# SPDX-License-Identifier: Apache-2.0
# *******************************************************************************
import re
from pathlib import Path
from unittest.mock import mock_open, patch

import pytest
from attribute_plugin import add_test_properties # type: ignore[import-untyped]
from score_metamodel import ProhibitedWordCheck, load_metamodel_data
from score_metamodel import (
GITHUB_TRACKING_LINK_REGEX,
ProhibitedWordCheck,
load_metamodel_data,
)

MODEL_DIR = Path(__file__).absolute().parent / "model"


@pytest.mark.parametrize(
("url", "expected_url", "expected_repo", "expected_number"),
[
(
"https://github.com/eclipse-score/docs-as-code/issues/1",
"https://github.com/eclipse-score/docs-as-code/issues/1",
"docs-as-code",
"1",
),
(
"https://github.com/eclipse-score/score/pull/2",
"https://github.com/eclipse-score/score/pull/2",
"score",
"2",
),
(
"https://github.com/eclipse-score/score/pull/2#discussion_r3",
"https://github.com/eclipse-score/score/pull/2",
"score",
"2",
),
],
)
def test_github_tracking_link_regex_matches_issue_pr_and_review_urls(
url: str, expected_url: str, expected_repo: str, expected_number: str
) -> None:
match = re.search(GITHUB_TRACKING_LINK_REGEX, url)

assert match is not None
assert match.groupdict() == {
"url": expected_url,
"repo": expected_repo,
"number": expected_number,
}


def test_github_tracking_link_regex_rejects_other_organizations() -> None:
assert (
re.search(
GITHUB_TRACKING_LINK_REGEX,
"https://github.com/other-org/score/pull/2",
)
is None
)


def load_model_data(model_file: str) -> str:
print(f"Loading model data from {model_file}")
model_path = Path(MODEL_DIR) / model_file
Expand Down
Loading