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.

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

YR/-BC-104-New-format-validation/CIAC-10001 #4187

Merged
merged 49 commits into from
Apr 14, 2024

Conversation

RosenbergYehuda
Copy link
Contributor

@RosenbergYehuda RosenbergYehuda commented Mar 27, 2024

Related Issues

fixes: https://jira-dc.paloaltonetworks.com/browse/CIAC-10001
fixes: https://jira-dc.paloaltonetworks.com/browse/CIAC-10114
fixes: https://jira-dc.paloaltonetworks.com/browse/CIAC-9694

Description

Convert half of BC 104 to the new format. This validation ensures that the names of existing commands or arguments have not been changed.
Furthermore, a new validation, BC110, was introduced following the split of BC104. This validation ensures that non-required arguments are not converted into required ones and that new arguments cannot be made mandatory.

Base automatically changed from YR/test-graph-was-created-successfully/CIAC-9913 to master March 27, 2024 13:56
@RosenbergYehuda RosenbergYehuda self-assigned this Apr 1, 2024
@RosenbergYehuda RosenbergYehuda requested review from YuvHayun and JasBeilin and removed request for dantavori and ilaner April 2, 2024 12:36
Copy link
Contributor

@YuvHayun YuvHayun left a comment

Choose a reason for hiding this comment

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

Good job!
See my comments.

Copy link
Contributor

@YuvHayun YuvHayun left a comment

Choose a reason for hiding this comment

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

Gj, minor comments.
@JasBeilin approved from my side.

Copy link
Contributor

@JasBeilin JasBeilin left a comment

Choose a reason for hiding this comment

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

great, think about making it simpler.

Copy link
Contributor

@JasBeilin JasBeilin left a comment

Choose a reason for hiding this comment

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

Great, See my comments.

@@ -54,7 +54,8 @@


# Create a new content item with 3 commands with unique names. all commands have only 1 argument except the third command which has 2 arguments.
generic_integration_with_3_commands_and_4_args = create_integration_object(

GENERIC_INTEGRATION_WITH_3_COMMANDS_AND_4_ARGS = create_integration_object(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the number of args & command important here? cant we call it just DUMMY_INTEGRATION?

Copy link
Contributor Author

@RosenbergYehuda RosenbergYehuda Apr 13, 2024

Choose a reason for hiding this comment

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

I think it helps to understand the test, especially when i am adding a new arg, so it is clear to know what did we have before and what we have now.

demisto_sdk/commands/validate/tools.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/tests/tools_test.py Outdated Show resolved Hide resolved
@RosenbergYehuda RosenbergYehuda merged commit 7691de3 into master Apr 14, 2024
23 of 24 checks passed
@RosenbergYehuda RosenbergYehuda deleted the YR/-BC-104-New-format-validation/CIAC-10001 branch April 14, 2024 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants