Platform-content-support_supported-modules-and-playbook-tags #4832
Conversation
barryyosi-panw
requested review from
dantavori and
RosenbergYehuda
as code owners
|
Changelog(s) in markdown:
|
|
Changelog(s) in markdown:
|
demisto_sdk/commands/common/content/objects/pack_objects/pack_metadata/pack_metadata.py
Outdated
Show resolved
Hide resolved
demisto_sdk/commands/common/content/objects/pack_objects/pack_metadata/pack_metadata.py
Outdated
Show resolved
Hide resolved
|
@thefrieddan1 @RosenbergYehuda |
…upported-modules-and-playbook-tags
Fixing unit tests
| Any: The same data object with supported modules appended. | ||
| """ | ||
|
|
||
| if isinstance(data, dict): |
There was a problem hiding this comment.
"""data (Any): The data to process, which can be a dictionary, list, or string."""
Why are we handling only dict?
There was a problem hiding this comment.
nice catch, I checked the type hints on the previous method that accepts this argument .
But self.data can only be a dict. Fixing
| # Replace incorrect marketplace references | ||
| data = replace_marketplace_references(data, current_marketplace, str(self.path)) | ||
| data = append_supported_modules(data, self.supportedModules) |
There was a problem hiding this comment.
Can you please explain this?
Why adding in to the object is not enough? because you need it in the bucket?
There was a problem hiding this comment.
yep - need to explicitly add it to the dumped .yml or .json
There was a problem hiding this comment.
yep - we inherit it from pack so we don't necessarily need to specify it on the content item level (unless we want to override the pack's value) - but server needs it explicitly, so we have to add it to the dumped content item.
| @@ -124,6 +125,7 @@ def upload_zip( | |||
|
|
|||
| class Pack(BaseContent, PackMetadata, content_type=ContentType.PACK): | |||
| path: Path | |||
| supportedModules: List[str] = DEFAULT_SUPPORTED_MODULES | |||
There was a problem hiding this comment.
Why do we need to add this if it was already added to BaseNode (which we inherit via BaseContent)? The only reason to do so would be if we plan to override it. Is that the case?
There was a problem hiding this comment.
yep - we would override it.
| git_sha: Optional[str] = None, | ||
| ) -> None: | ||
| super().__init__(path, pack_marketplaces, git_sha=git_sha) | ||
| super().__init__( |
There was a problem hiding this comment.
Same question here—after defining it in ContentItemParser, why do we need to do the same for all its inheritors? Won't it already be inherited?
This is the same question for all inherited classes.
There was a problem hiding this comment.
we inherit it both from Base Node in (OOP wise), but, we also need to be able to inherit this from the pack level - so its used while parsing the pack's content items.
barryyosi-panw
deleted the
platform-content-support_supported-modules-and-playbook-tags
branch
Related Issues
fixes: CIAC-12869
Description