(PDK-421) Validate EPP syntax #680
Conversation
|
Hi @raphink! Thanks for the contribution! We'll get someone to take a look at this as soon as we can, in the meantime the test failures look like they should be pretty easy to address and the coveralls failure is just asking for this new code to have some unit test coverage. We can look at addressing that ourselves but if you wanted to work on fixing up those failures it would help us get this reviewed and merged more quickly. :) Thanks again! |
scotje
added
feature
Needs Tests
needs-triage
There was a problem hiding this comment.
Thanks @raphink . Given that the epp validator is so similiar to the puppet-syntax validator, could you also create the unit tests as well?
|
Just added some unit tests. I was wondering about the Puppet version contexts, but it does actually impact the EPP validation output as well. |
|
The tests are passing now. Do you want me to add something still @glennsarti ? |
|
@raphink Sorry for the delayed response. We're just in the middle of a PDK release, but I promise I'll get back to you soon. |
| let(:options) { { auto_correct: true } } | ||
|
|
||
| it 'has no effect' do | ||
| expect(command_args).to eq(%w[epp validate --config /dev/null --modulepath].push(tmpdir).concat(targets)) |
There was a problem hiding this comment.
Note for me, not to stop merging: Need to investigate why using a fixed Linux path. This shouldn't work on Windows?
|
@rodjek Apart from removing one test I'm happy to merge this. Do you want to review prior? |
scotje
removed
Needs Tests
needs-triage
|
Done removing the extra test @scotje |
|
All concerns have been addressed. Merging. Thanks again @raphink !! |
rodjek
changed the title
This is a base implementation for validating EPP templates. I essentially copy/pasted the Puppet validate one, so it probably needs some pruning, but it works.