Skip to content
This repository has been archived by the owner on Nov 29, 2021. It is now read-only.

Start with param #31

Merged
merged 5 commits into from
Jul 12, 2018
Merged

Start with param #31

merged 5 commits into from
Jul 12, 2018

Conversation

jjnicola
Copy link
Member

Add vts params to start_scan as a vt subelement.
Now each vt is a subelement of . It has an id attribute. Each vt accept multiple optional vt_param as subelements, with name and type as attribute and the value as text.
Update example for start_scan with vts parameters.
Add unittest to test VTs as elements and VT's parameters.

jjnicola added 3 commits July 10, 2018 15:30
Now each vt is a subelement of <vts>. It has an id attribute.
Each vt accept multiple optional vt_param as subelements, with
name and type as attribute and the value as text.
Update example for start_scan with vts parameters
@jjnicola jjnicola requested review from janowagner and a team July 11, 2018 10:36
Copy link
Member

@janowagner janowagner left a comment

Choose a reason for hiding this comment

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

I see three things to improve:

  1. The use of variable "id", see the CI error.
  2. Don't use ET.fromstring even in the tests module,
    like I also mentioned in the other PR where secET
    was introduced (partially).
  3. "process something" is neither a helpful
    command name and even more it is not helpful
    as a function documentation. Please explain in
    more detail in the doc string what "process it"
    actually means.

@jjnicola jjnicola mentioned this pull request Jul 12, 2018
@jjnicola
Copy link
Member Author

I have done the improvements 1) and 3) . Regarding the point 2), as I explained in PR #30, I will do a new PR with the requested changes for the tests, to avoid merge conflicts.

@jjnicola jjnicola requested a review from janowagner July 12, 2018 11:40
Copy link
Member

@janowagner janowagner left a comment

Choose a reason for hiding this comment

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

Thanks for the 2 solved items.
So, lets approve this PR and as a follow-up then
use secure XML parsing in the tests.

@janowagner janowagner merged commit a8c94d4 into greenbone:master Jul 12, 2018
@jjnicola jjnicola deleted the start-with-param branch July 13, 2018 08:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants