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.

Sign up for GitHub

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

fix(project): Use an annotated type for duration strings #5213

Open
wants to merge 4 commits into
base: hotfix/8.6
Choose a base branch
from
Open

fix(project): Use an annotated type for duration strings #5213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d230ff3
fix(project): Use an annotated type for duration strings
lengau Jan 21, 2025
139a054
fix: update schema
lengau Jan 21, 2025
12d16bf
fix: better description
lengau Jan 21, 2025
1f47e37
Merge branch 'hotfix/8.6' into work/better-duration-regex
mr-cal Jan 27, 2025
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
8 changes: 4 additions & 4 deletions schema/snapcraft.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,19 +711,19 @@
},
"start-timeout": {
"type": "string",
"pattern": "^[0-9]+(ns|us|ms|s|m)*$",
"pattern": "^([0-9]+(ns|us|ms|s|m)){1,5}$",
"validation-failure": "{.instance!r} is not a valid timeout value.",
"description": "Optional time to wait for daemon to start - <n>ns | <n>us | <n>ms | <n>s | <n>m"
},
"stop-timeout": {
"type": "string",
"pattern": "^[0-9]+(ns|us|ms|s|m)*$",
"pattern": "^([0-9]+(ns|us|ms|s|m)){1,5}$",
"validation-failure": "{.instance!r} is not a valid timeout value.",
"description": "Optional time to wait for daemon to stop - <n>ns | <n>us | <n>ms | <n>s | <n>m"
},
"watchdog-timeout": {
"type": "string",
"pattern": "^[0-9]+(ns|us|ms|s|m)*$",
"pattern": "^([0-9]+(ns|us|ms|s|m)){1,5}$",
"validation-failure": "{.instance!r} is not a valid timeout value.",
"description": "Service watchdog timeout - <n>ns | <n>us | <n>ms | <n>s | <n>m"
},
Expand All @@ -733,7 +733,7 @@
},
"restart-delay": {
"type": "string",
"pattern": "^[0-9]+(ns|us|ms|s|m)*$",
"pattern": "^([0-9]+(ns|us|ms|s|m)){1,5}$",
"validation-failure": "{.instance!r} is not a valid delay value.",
"description": "Delay between service restarts - <n>ns | <n>us | <n>ms | <n>s | <n>m. Defaults to unset. See the systemd.service manual on RestartSec for details."
},
Expand Down
37 changes: 23 additions & 14 deletions snapcraft/models/project.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
from snapcraft.providers import SNAPCRAFT_BASE_TO_PROVIDER_BASE
from snapcraft.utils import get_effective_base

TIME_DURATION_REGEX = re.compile(r"^([0-9]+(ns|us|ms|s|m)){1,5}$")
ProjectName = Annotated[str, StringConstraints(max_length=40)]


Expand Down Expand Up @@ -282,6 +283,24 @@ def _validate_mandatory_base(base: str | None, snap_type: str | None) -> None:
)


def _validate_duration_string(duration: str):
if not TIME_DURATION_REGEX.match(duration):
raise ValueError(f"{duration!r} is not a valid time value")

return duration


DurationString = Annotated[
str,
pydantic.Field(
examples=["1", "2s", "3m", "4ms", "5us", "6m7s8ms"],
pattern=TIME_DURATION_REGEX,
description="A duration string to be parsed by snapd.",
),
pydantic.BeforeValidator(_validate_duration_string),
]


class Socket(models.CraftBaseModel):
"""Snapcraft app socket definition."""

Expand Down Expand Up @@ -415,17 +434,17 @@ class App(models.CraftBaseModel):
description="The command to run after the service is stopped.",
examples=["post-stop-command: bin/logrotate --force"],
)
start_timeout: str | None = pydantic.Field(
start_timeout: DurationString | None = pydantic.Field(
default=None,
description="The maximum amount of time to wait for the service to start.",
examples=["start-timeout: 10s", "start-timeout: 2m"],
)
stop_timeout: str | None = pydantic.Field(
stop_timeout: DurationString | None = pydantic.Field(
default=None,
description="The maximum amount of time to wait for the service to stop.",
examples=["stop-timeout: 10s", "stop-timeout: 2m"],
)
watchdog_timeout: str | None = pydantic.Field(
watchdog_timeout: DurationString | None = pydantic.Field(
default=None,
description="The maximum amount of time the service can run without sending a heartbeat to the watchdog.",
examples=["watchdog-timeout: 10s", "watchdog-timeout: 2m"],
Expand All @@ -435,7 +454,7 @@ class App(models.CraftBaseModel):
description="The command to run to restart the service.",
examples=["reload-command: bin/foo-app --restart"],
)
restart_delay: str | None = pydantic.Field(
restart_delay: DurationString | None = pydantic.Field(
default=None,
description="The time to wait between service restarts.",
examples=["restart-delay: 10s", "restart-delay: 2m"],
Expand Down Expand Up @@ -588,16 +607,6 @@ def _validate_apps_section_content(cls, command: str) -> str:

return command

@pydantic.field_validator(
"start_timeout", "stop_timeout", "watchdog_timeout", "restart_delay"
)
@classmethod
def _validate_time(cls, timeval):
if not re.match(r"^[0-9]+(ns|us|ms|s|m)*$", timeval):
raise ValueError(f"{timeval!r} is not a valid time value")

return timeval

@pydantic.field_validator("command_chain")
@classmethod
def _validate_command_chain(cls, command_chains):
Expand Down
38 changes: 10 additions & 28 deletions tests/unit/models/test_projects.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@

# required project data for core24 snaps
CORE24_DATA = {"base": "core24", "grade": "devel"}
VALID_DURATIONS = ["10ns", "10us", "10ms", "10s", "10m", "10m4s3us"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a separate data entry for a valid duration here, as "10m4s3us" is a valid duration string. Snapd uses time.ParseDuration to parse this, and the resulting service file contains a value something like TimeoutStartSec=604.000003.

INVALID_DURATIONS = ["10", "10 s", "10 seconds", "1:00", "invalid"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Surprisingly, just an integer isn't a valid duration. The current snapcraft will error out during the packing step:

$ snapcraft_edge pack
Generated snap metadata
Cannot pack snap: error: cannot parse snap.yaml: time: missing unit in duration "10"
Detailed information: Command '['snap', 'pack', '--check-skeleton', PosixPath('/root/prime')]' returned non-zero exit status 1.
Failed to execute pack in instance.
Recommended resolution: Run the same command again with --debug to shell into the environment if you wish to introspect this failure.
Full execution log: '/home/lengau/.local/state/snapcraft/log/snapcraft-20250121-181707.415584.log'



@pytest.fixture
Expand Down Expand Up @@ -982,59 +984,44 @@ def test_app_post_stop_command(self, app_yaml_data):
assert project.apps is not None
assert project.apps["app1"].post_stop_command == "test-post-stop-command"

@pytest.mark.parametrize(
"start_timeout", ["10", "10ns", "10us", "10ms", "10s", "10m"]
)
@pytest.mark.parametrize("start_timeout", VALID_DURATIONS)
def test_app_start_timeout_valid(self, start_timeout, app_yaml_data):
data = app_yaml_data(start_timeout=start_timeout)
project = Project.unmarshal(data)
assert project.apps is not None
assert project.apps["app1"].start_timeout == start_timeout

@pytest.mark.parametrize(
"start_timeout",
["10 s", "10 seconds", "1:00", "invalid"],
)
@pytest.mark.parametrize("start_timeout", INVALID_DURATIONS)
def test_app_start_timeout_invalid(self, start_timeout, app_yaml_data):
data = app_yaml_data(start_timeout=start_timeout)

error = f"'{start_timeout}' is not a valid time value"
with pytest.raises(pydantic.ValidationError, match=error):
Project.unmarshal(data)

@pytest.mark.parametrize(
"stop_timeout", ["10", "10ns", "10us", "10ms", "10s", "10m"]
)
@pytest.mark.parametrize("stop_timeout", VALID_DURATIONS)
def test_app_stop_timeout_valid(self, stop_timeout, app_yaml_data):
data = app_yaml_data(stop_timeout=stop_timeout)
project = Project.unmarshal(data)
assert project.apps is not None
assert project.apps["app1"].stop_timeout == stop_timeout

@pytest.mark.parametrize(
"stop_timeout",
["10 s", "10 seconds", "1:00", "invalid"],
)
@pytest.mark.parametrize("stop_timeout", INVALID_DURATIONS)
def test_app_stop_timeout_invalid(self, stop_timeout, app_yaml_data):
data = app_yaml_data(stop_timeout=stop_timeout)

error = f"'{stop_timeout}' is not a valid time value"
with pytest.raises(pydantic.ValidationError, match=error):
Project.unmarshal(data)

@pytest.mark.parametrize(
"watchdog_timeout", ["10", "10ns", "10us", "10ms", "10s", "10m"]
)
@pytest.mark.parametrize("watchdog_timeout", VALID_DURATIONS)
def test_app_watchdog_timeout_valid(self, watchdog_timeout, app_yaml_data):
data = app_yaml_data(watchdog_timeout=watchdog_timeout)
project = Project.unmarshal(data)
assert project.apps is not None
assert project.apps["app1"].watchdog_timeout == watchdog_timeout

@pytest.mark.parametrize(
"watchdog_timeout",
["10 s", "10 seconds", "1:00", "invalid"],
)
@pytest.mark.parametrize("watchdog_timeout", INVALID_DURATIONS)
def test_app_watchdog_timeout_invalid(self, watchdog_timeout, app_yaml_data):
data = app_yaml_data(watchdog_timeout=watchdog_timeout)

Expand All @@ -1048,19 +1035,14 @@ def test_app_reload_command(self, app_yaml_data):
assert project.apps is not None
assert project.apps["app1"].reload_command == "test-reload-command"

@pytest.mark.parametrize(
"restart_delay", ["10", "10ns", "10us", "10ms", "10s", "10m"]
)
@pytest.mark.parametrize("restart_delay", VALID_DURATIONS)
def test_app_restart_delay_valid(self, restart_delay, app_yaml_data):
data = app_yaml_data(restart_delay=restart_delay)
project = Project.unmarshal(data)
assert project.apps is not None
assert project.apps["app1"].restart_delay == restart_delay

@pytest.mark.parametrize(
"restart_delay",
["10 s", "10 seconds", "1:00", "invalid"],
)
@pytest.mark.parametrize("restart_delay", INVALID_DURATIONS)
def test_app_restart_delay_invalid(self, restart_delay, app_yaml_data):
data = app_yaml_data(restart_delay=restart_delay)

Expand Down
Loading