Change plugin config to expect optional minimum lua version string #122
Conversation
jeffngo
requested review from
ankursarin,
fdrozdowski,
mothslaw,
nhlien93 and
ravi-cm
...s/src/main/python/dlpx/virtualization/_internal/validation_schemas/plugin_config_schema.json
Outdated
Show resolved
Hide resolved
.../dlpx/virtualization/_internal/validation_schemas/plugin_config_schema_no_id_validation.json
Outdated
Show resolved
Hide resolved
nhlien93
left a comment
nhlien93
left a comment
There was a problem hiding this comment.
are you planning to add the changes we discussed where we validate that if a luaName exist the minimumLuaVersion also must exist? Also i think we'd wanna do it the other way around too? Where if minimumLuaVersion exists luaName must also exist.
...s/src/main/python/dlpx/virtualization/_internal/validation_schemas/plugin_config_schema.json
Outdated
Show resolved
Hide resolved
.../dlpx/virtualization/_internal/validation_schemas/plugin_config_schema_no_id_validation.json
Outdated
Show resolved
Hide resolved
|
oh I realized after I posted that you hadn't actually updated your diff (idk why I thought you did) Ignore me! |
… strings. Added a check (per Lindsey's suggestion) to ensure that luaName and minimumLuaVersion are both set.
tools/src/main/python/dlpx/virtualization/_internal/commands/build.py
Outdated
Show resolved
Hide resolved
nhlien93
left a comment
nhlien93
left a comment
There was a problem hiding this comment.
agreed with what ravi said with validation. we should try to throw user errors like this only in the validation code to be consistent.
tools/src/test/python/dlpx/virtualization/_internal/conftest.py
Outdated
Show resolved
Hide resolved
tools/src/test/python/dlpx/virtualization/_internal/conftest.py
Outdated
Show resolved
Hide resolved
ravi-cm
left a comment
ravi-cm
left a comment
There was a problem hiding this comment.
Thank you for the new changes, looks good.
nhlien93
left a comment
nhlien93
left a comment
There was a problem hiding this comment.
looks good to me.
could you link the doc bugs that are related to making sure we explicitly call out that the major/minor value is what the version is? just so we're tracking certain things and can always go back to this if we forget why we need the bug.
4 participants
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces:
What is the current and new behavior?
Plugin authors might feel the need to deprecate older versions of LUA toolkits at some point in the future. To aid plugin authors on this endeavor, we will allow them to specify the lowest version of a LUA toolkit that can be upgraded. Right now, the plugin config does not expect a field to denote the minimum lua version that the plugin can upgrade. This change adds that field.
Does this introduce a breaking change?
Other information