-
Notifications
You must be signed in to change notification settings - Fork 14
Manifests migration #5575
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
base: main
Are you sure you want to change the base?
Manifests migration #5575
Conversation
|
|
3addd02 to
84b9daa
Compare
84b9daa to
aab6bb4
Compare
7a87ca7 to
e3c5f7a
Compare
e3c5f7a to
46e60b9
Compare
6647ba8 to
3e9fcbb
Compare
| @@ -1,129 +0,0 @@ | |||
| import os | |||
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.
Why this file has been removed ?
| self.parse_declaration() | ||
|
|
||
| @staticmethod | ||
| def fix_separator(version: str) -> str: |
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.
I feel like we should be more strict with the content of manifest, less complexity, and cleaner data.
Do we have lot of use case to fix ?
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.
I agree but we won't be able to fix the versioning in the libraries themselves and having a different version syntax in the manifests compared to the rest of the lib repo could be confusing
| from .types import ManifestData, Condition, SkipDeclaration | ||
|
|
||
|
|
||
| def match_condition( |
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.
This function should a method of Condition, no ?
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.
It could but it is never used outside of match_rule
| path = rule.split("/") | ||
| path = [part for part in path if part] | ||
| rest = rule.split("::") | ||
| rule_elements = [*path[:-1], path[-1].split("::")[0], *rest[1:]] |
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.
Can be done with rule_elements = Path(rule.replace("::", "/")).parts
pathlib.Path is a great tool to manipulate paths.
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.
rule_elements = rule.replace("::", "/").split("/") would also work no ?
utils/manifest/_internal/rule.py
Outdated
| if len(rule_elements) > len(nodeid_elements): | ||
| return False | ||
| return all(elements[0] == elements[1] for elements in zip(rule_elements, nodeid_elements, strict=False)) |
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.
| if len(rule_elements) > len(nodeid_elements): | |
| return False | |
| return all(elements[0] == elements[1] for elements in zip(rule_elements, nodeid_elements, strict=False)) | |
| return nodeid_elements[:len(rule_elements)] == rule_elements |
| force_skip: bool = False, | ||
| ): | ||
| if not inspect.isfunction(item) and not inspect.isclass(item) and not isinstance(item, pytest.Module): | ||
| if ( |
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.
IIUC, this block should be isinstance(item, pytest.Item) only ?
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.
mmmh, no, it's can be called b decorator, so it's the prototype that should be kept to pytest.Item | FunctionType | MethodType
Motivation
Changes
Workflow
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
[<language>], double-check that only<language>is impacted by the changebuild-XXX-imagelabel is present