IVCTool / IVCT_Framework

For IVCT Framework Developers. Core System for the IVCT (Integration, Verification and Certification Tool) for HLA Federates
Apache License 2.0
16 stars 4 forks source link

Handling of common TC parameters #181

Closed rhzg closed 5 years ago

rhzg commented 5 years ago

some parameters. like the federation name or RTI address, are defined in multible locations. This should be done in a more efficient way

rhzg commented 5 years ago

policy for handling the common parameters, like CRC adress and port, federation name, etc.

rhzg commented 5 years ago

It should also not be handled in the test case implementation.

Order for settings designator should be

  1. LRC base container settings are processed, that will prepare all RTI configuration files.
  2. then a test case is started with the tc specific parameter file (by default, this settings should be empty, meaning you are using the option 1 but you are able to overwrite through the GUI)
bergtwvd commented 5 years ago

Proposed logic for processing common parameters:

1 - Get parameter value from parameter file. If not set or the value is an empty string, then the value is assumed empty. 2 - If the value from 1 is empty then get parameter value from environment variable (EV). The EV may be unset or set to an empty string, so the parameter value may still be empty. 3 - if the value from 2 is empty then use a pre-determined default value.

Parameter Name Environment variable Predetermined default Semantics on empty
FederationName FEDERATIONNAME TheWorld Error, must be set
FederateName FEDERATENAME empty Name determined by the RTI
FederateType FEDERATETYPE IVCT Error, must be set
SettingsDesignator SETTINGS_DESIGNATOR empty Defaults to what has been set in RTI configuration files (see step 1)
bergtwvd commented 5 years ago

Adding to the previous comment, best is to do the processing in reverse order: 1 - set default value from table 2 - if EV is non-empty then use that 3 - if parm in file is non-empty then use that

bergtwvd commented 5 years ago

Maybe step 2 and 3 should be swapped. That way the config is alays controllable from the environment. Discuss....

rhzg commented 5 years ago

To me the logical order would be the following (more like your first proposal):

  1. Default values should be within the test case code. If nothing is provided, every value need have a meaningful default value.
  2. The IVCT installation may/or may not have installation-specific defaults, overwriting the level 1 default. That may be useful for things like the RTI address, or maybe proxy settings. This second default level can be defined via a properties file which may be overwritten by environment settings.
  3. The actual values will be defined in the tc.param settings. This can be a complete definition of all parameters, in which case none of the default levels are used. If the default settings shall be used, the respective parameters need to be removed from the tc.param setting.

But my concern is, that this handling needs to be apparent to the user. If the default values are not visible within the user interface, it is quite error-prone. We need to extend the GUI for that, to make it fail-save.

I propose to address this issue in Release 3.0, in order to be able to close release 2.0 before the upcoming CWIX. This is also related to our general storage concepts for SuT’s, badge test results, test suite parameter settings. And this need some major revision, as currently we have a not quite consistent interpretation of badge and test-suite.

bergtwvd commented 5 years ago

So, if I read correctly, the proposed initialization order for the TC params is:

  1. apply the value provided in TC parm file

  2. is set, apply the value provided via EV, overriding the value provided in the previous step

I think this is still the same proposal as described above, where step 0 is: apply predetermined default value (see table in earlier comment).

I assume that the TC params (the result of applying all of the steps) is presented to the user. E.g. if no value is set in the TC parm file, and no value is set in the EV, then the pre-determined default is shown to the user. This does not require changes to the GUI I believe, only the underlying logic to apply the steps above should be added.

The importance for me is to have the capability to set PC param values via the EV. E.g. the Settings Designator. Once TC params are hard-wired in the file, it is hard to change them (from container point of view).

rhzg commented 5 years ago

yes, correct. The steps are like your first proposal. My point was, that is see environment variables as more generic and related to the tool installation, while tc.params are more specifically related to one individual test case execution, and therefore should overwrite all other default or generic values.

Currently our GUI only reads and uses the TC.param file and shows this one to the user. If we overwrite values within this file, we need to extend the GUI to implement this logic. If your use case is the headless mode, maybe we could think of a script-based scheduler, instead of adding this to the GUI?

bergtwvd commented 5 years ago

Maybe we should split the issue in (a) agree on the set of common parameters and their default value (this set is by definition always present in the TC parms file, and the value of each parm is adaptable via the UI), (b) make the common parameters adaptable via EVs.

Then (a) is for release 2 (so we can square that away), and (b) for release 3.

W.r.t. (a): In the UI I see a set of params, e.g. RtiHostname. This particular one is RTI specific and should be replaced by RtiSettingsDesignator. Same for RtiPort. Do we need SutFederateName? (you can get this from the RTI)

bergtwvd commented 5 years ago

While reorganizing container documentation I came across the handling of the Settings Designator in https://github.com/MSG134/IVCT_Framework/wiki/IVCT-TC-Runner-Application-image. Here the text assumes that the designator can be set via an EV. If we only go for option (a) for release 2.0, then this is not valid for the TC Runner component. This text needs to be relocated.

rhzg commented 5 years ago

I just realized we already have the logic in place that uses an environment variable, that overwrites the rtiHost and rtiPort settings within the TC.param:

SETTINGS_DESIGNATOR=crcAddress=localhost:8989

I was not aware of that and I need to check how this works with the HLA test suites

bergtwvd commented 5 years ago

Yes, would be great if this works! (although the above listed value is Pitch RTI specific - default should be empty).

rhzg commented 5 years ago

yes, the default in the code is empty. My problem with that is, that this variable is set within the TC-runner and there is currently not way for the GUI to see its value. I think this is unaccetable for the user

bergtwvd commented 5 years ago

In my view the TC parm file has several required parameters (see list of parameter names several posts back). I expect that these will appear in the GUI alongside with any additional TC parms in the file. Since we know that these are required, these parms can have special "treatment" in the GUI, e.g. list them in a separate panel, or make them bold, ...

rhzg commented 5 years ago

That requires to define the parameters in the UI or GUI container and pass them within the tc-run-command to the tc-runner. Fine with my.

But first I need to fix the issue with the TC.lib https://github.com/MSG134/IVCT_Framework/issues/161 otherwise we have a version mess with our test suites.

bergtwvd commented 5 years ago

Yes, that would be step (a) from several posts back:

(a) agree on the set of common parameters and their default value (this set is by definition always present in the TC parms file, and the value of each parm is adaptable via the UI), (b) make the common parameters adaptable via EVs.

(a) is for release 2.0 and (b) for release 3.0.

Think we are in agreement :-).

rhzg commented 5 years ago

settingsDesignator and federationName is not included in the conformance statement of an SuT and can be edited within the GUI

bergtwvd commented 5 years ago

The documentation (also for the container images) needs to be updated to reflect this.