-
Notifications
You must be signed in to change notification settings - Fork 499
feat(metadata-linter): impl new linter #5610
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
Conversation
20dac75 to
13cc377
Compare
lengau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! I think this is great at a high level - just some implementation nits :-)
5272796 to
5b6697c
Compare
mr-cal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, this is looking good. I've left some feedback from testing.
We're missing this part of the task:
New documentation on how to manage warnings from the metadata linter
You can refer to the existing documentation for the classic and library linters.
Can you also add a new row to this table for the common-id? Your new entry in the table should link upstream.
tests/spread/core24/linters-pack/metadata-ignore-fields/expected_linter_output.txt
Outdated
Show resolved
Hide resolved
tests/spread/core24/linters-pack/metadata-ignore-fields/expected_linter_output.txt
Outdated
Show resolved
Hide resolved
5b6697c to
2fe8846
Compare
2fe8846 to
8def81c
Compare
8def81c to
1ffea47
Compare
|
class field MetadataField(
"common_id",
LinterResult.WARNING,
lambda meta: _get_apps_attr(meta, "common_id"),
f"{_HELP_URL}#apps.<app-name>.common-id",
lambda meta: bool((meta.type and meta.type != "app") or not meta.apps),
),
helper to get the attrs # Helper to pull the matching attribute values from all the apps
def _get_apps_attr(meta: SnapMetadata, key: str) -> dict[str, list[str]] | None:
if not meta.apps:
return None
values: dict[str, list[str]] = {}
for name, app in meta.apps.items():
value: str | list[str] | None = getattr(app, key, None)
if value and isinstance(value, list):
values[name] = value
elif isinstance(value, str):
values[name] = [value]
else:
values[name] = []
return values if values else NoneCondition to get the elif isinstance(result, dict):
for name, is_empty in result.items():
if is_empty:
issues.append(
self._create_issue(
field,
f"Metadata field '{field_name}' for app '{name}' is missing or empty.",
)
)Helper for validation @classmethod
def _is_dict_empty(
cls, value: dict[str, list[str]] | None
) -> dict[str, bool] | bool:
"""Check if dictionary values are empty or None.
:param value: Dictionary to check
:returns: True if all values are empty, Dict mapping keys to empty status otherwise
"""
if value is None:
return True
result = {}
for key, values in value.items():
if not values:
result[key] = True
return result if result else FalseUnit Tests def test_metadata_linter_common_id(new_dir, snap_yaml_data):
yaml_data = snap_yaml_data(**{"apps": {"app1": {"command": "bin/mytest"}}})
project = models.Project.unmarshal(yaml_data)
snap_yaml.write(project, prime_dir=Path(new_dir), arch="amd64")
issues = linters.run_linters(new_dir, lint=None)
assert issues[2] == LinterIssue(
name="metadata",
result=LinterResult.WARNING,
text="Metadata field 'common-id' for app 'app1' is missing or empty.",
url="https://documentation.ubuntu.com/snapcraft/stable/reference/project-file/snapcraft-yaml/#apps.<app-name>.common-id",
)
def test_metadata_linter_common_id_with_multiple_app(new_dir, snap_yaml_data):
yaml_data = snap_yaml_data(
**{
"apps": {
"app1": {"command": "bin/mytest", "common_id": "com.example.mytest"},
"app2": {"command": "bin/mytest"},
}
}
)
project = models.Project.unmarshal(yaml_data)
snap_yaml.write(project, prime_dir=Path(new_dir), arch="amd64")
issues = linters.run_linters(new_dir, lint=None)
assert issues[2] == LinterIssue(
name="metadata",
result=LinterResult.WARNING,
text="Metadata field 'common-id' for app 'app2' is missing or empty.",
url="https://documentation.ubuntu.com/snapcraft/stable/reference/project-file/snapcraft-yaml/#apps.<app-name>.common-id",
)
def test_metadata_linter_empty_common_id_in_multiple_app(new_dir, snap_yaml_data):
yaml_data = snap_yaml_data(
**{
"apps": {
"app1": {"command": "bin/mytest"},
"app2": {"command": "bin/mytest"},
}
}
)
project = models.Project.unmarshal(yaml_data)
snap_yaml.write(project, prime_dir=Path(new_dir), arch="amd64")
issues = linters.run_linters(new_dir, lint=None)
assert issues[2] == LinterIssue(
name="metadata",
result=LinterResult.WARNING,
text="Metadata field 'common-id' for app 'app1' is missing or empty.",
url="https://documentation.ubuntu.com/snapcraft/stable/reference/project-file/snapcraft-yaml/#apps.<app-name>.common-id",
)
assert issues[3] == LinterIssue(
name="metadata",
result=LinterResult.WARNING,
text="Metadata field 'common-id' for app 'app2' is missing or empty.",
url="https://documentation.ubuntu.com/snapcraft/stable/reference/project-file/snapcraft-yaml/#apps.<app-name>.common-id",
)
def test_metadata_linter_ignore_common_id_when_not_app(new_dir, snap_yaml_data):
yaml_data = snap_yaml_data(
**{"apps": {"app2": {"command": "bin/mytest"}}, "type": "gadget"}
)
project = models.Project.unmarshal(yaml_data)
snap_yaml.write(project, prime_dir=Path(new_dir), arch="amd64")
issues = linters.run_linters(new_dir, lint=None)
assert len(issues) == 5 |
0f81fc8 to
fbb8d57
Compare
mr-cal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just feedback about the style at this point.
fbb8d57 to
7c91c62
Compare
|
All done! |
b43f7ef to
bfc22cf
Compare
bfc22cf to
75f0ebd
Compare
75f0ebd to
2ff6b8e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a new metadata linter for Snapcraft that checks for missing or empty metadata fields and reports them with appropriate severity levels. The linter validates snap metadata completeness to improve store listings and follows a rank-based system where critical fields generate warnings and optional fields generate informational messages.
Key changes:
- Added a new
MetadataLinterclass that validates snap metadata fields with rank-based severity - Added
INFOresult type to the linter framework for informational issues - Extended test coverage with unit tests and spread tests for various scenarios
Reviewed Changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
snapcraft/linters/metadata_linter.py |
Core implementation of the metadata linter with field validation logic |
snapcraft/linters/base.py |
Added new INFO linter result type |
snapcraft/linters/linters.py |
Registered metadata linter and added INFO status handling |
tests/unit/linters/test_metadata_linter.py |
Comprehensive unit tests for metadata linter functionality |
| Multiple spread test files | Integration tests covering various metadata linter scenarios |
| Documentation files | Added usage documentation and updated linter reference |
|
We're scheduled to discuss this internally on Friday (#5610 (comment)), so we should have another review on Friday. |
mr-cal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 minor changes, otherwise this looks good to me
mr-cal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
We can land this once @medubelko re-reviews the docs.
medubelko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a handful of suggestions for small improvements, but overall this looks good. Thanks @soumyaDghosh!
c1312e9 to
a311d1d
Compare
1. added the new linter 2. lints based on ranks 3. added unit tests and spread tests for the new linter 4. added a new LinterResult value named INFO 5. fix spread tests accordingly 6. add docs regarding the new metadata linter Signed-off-by: Soumyadeep Ghosh <[email protected]>
Signed-off-by: Soumyadeep Ghosh <[email protected]>
a311d1d to
abf22a9
Compare
Signed-off-by: Soumyadeep Ghosh <[email protected]>
Signed-off-by: Soumyadeep Ghosh <[email protected]>
abf22a9 to
251b69e
Compare
|
@mr-cal This should be good to go. |
Signed-off-by: Soumyadeep Ghosh <[email protected]> Co-authored-by: Callahan Kovacs <[email protected]>
make lint?make test?closes #5544 #5155