Feature/load new lifecycle config#208
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
| module_name = "score_baselibs", | ||
| commit = "498a4b256c9073602140243d30c33b106e279f75", | ||
| remote = "https://github.com/eclipse-score/baselibs.git", | ||
| ) |
There was a problem hiding this comment.
@paulquiring Any idea when a new baselibs release will be available containing the new flatbuffer functionality?
There was a problem hiding this comment.
Next release of baselibs: planed on 21.05 or 22.05. See https://github.com/orgs/eclipse-score/discussions/2390?sort=new#discussioncomment-16987333
| } | ||
|
|
||
| private: | ||
| friend class Builder; |
There was a problem hiding this comment.
A comment would have been helpful to mention that this saves the need to write getters in the builder class.
There was a problem hiding this comment.
Discussed in meeting. The friend is needed to access the private constructor of the Config class.
This is in line with typical builder pattern implementation
There was a problem hiding this comment.
Moving the Builder class to the outer scope so that it is not a nested class anymore.
Nested class is not required and may be discouraged by some SCA rules.
Done in f70d64b
| { | ||
| if (ev != nullptr && ev->key() != nullptr) | ||
| { | ||
| result.emplace(ev->key()->str(), safeString(ev->value())); |
There was a problem hiding this comment.
I guess the key cannot be nullptr?
There was a problem hiding this comment.
In the flatbuffer schema I have defined it as
// A key-value pair representing an environment variable.
table EnvironmentalVariable {
key:string (required);
value:string (required);
}
So I think if an environment variable is there, the key and value must be available.
There was a problem hiding this comment.
Consider adding asserts for values that are never expected to be nullptr.
safestring can be removed for value() because it is required in the schema
|
|
||
| SwitchRunTargetAction convertRequiredSwitchRunTargetAction(const fb::SwitchRunTargetAction* sa) | ||
| { | ||
| return convertSwitchRunTargetAction(sa).value(); |
There was a problem hiding this comment.
It should never happen because the SwitchRunTargetRecoveryAction is defined as required in this context:
// Configuration for a Run Target representing an operational mode of the system.
table RunTarget {
....
// Recovery action when a component in this Run Target fails.
recovery_action:SwitchRunTargetAction (required);
}
However to be defensive against potential future changes on the schema, we could:
- Add a try/catch within parseFlatbuffer and return
score::cpp::make_unexpected(IConfigLoader::Error::InvalidFormat);in case there is any exception. - And/or put in an assertion
auto opt=convertSwitchRunTargetAction(sa)
assert(opt.has_value() && "SwitchRunTargetAction is null although it is expected to be a mandatory configuration");
return opt.value();
What do you think?
There was a problem hiding this comment.
Discussed: We shall add asserts
Done in 99cdb9c
| module_name = "score_baselibs", | ||
| commit = "498a4b256c9073602140243d30c33b106e279f75", | ||
| remote = "https://github.com/eclipse-score/baselibs.git", | ||
| ) |
There was a problem hiding this comment.
Next release of baselibs: planed on 21.05 or 22.05. See https://github.com/orgs/eclipse-score/discussions/2390?sort=new#discussioncomment-16987333
| constexpr int32_t kExpectedSchemaVersion = 1; | ||
| constexpr double kSecondsToMilliseconds = 1000.0; | ||
|
|
||
| uint32_t secondsToMs(double seconds) |
There was a problem hiding this comment.
#include <iostream>
#include <cstdint>
#include <string_view>
#include <cassert>
constexpr double kSecondsToMilliseconds = 1000.0;
uint32_t secondsToMs(double seconds)
{
return static_cast<uint32_t>(seconds * kSecondsToMilliseconds);
}
int main () {
assert(secondsToMs(0.000500) == 0);// holds is this a problem ?
assert(secondsToMs(0.001000) == 1);
}if this is a problem we might need to change the fbs
There was a problem hiding this comment.
I guess this is not an issue.
The user is configuring all time values in seconds
Though we should reconsider whether the want to switch the configuration to ms.
There was a problem hiding this comment.
How to safeguard that the user does not configure anything < 1ms?
There was a problem hiding this comment.
Adding an assertion for now, to be moved to schema validation later
There was a problem hiding this comment.
Assertion added in a1f2860
Assertion fails if configured seconds > 0 but is rounded down to 0ms.
|
review from my side is finished. |
|
|
||
| uint32_t secondsToMs(double seconds) | ||
| { | ||
| return static_cast<uint32_t>(seconds * kSecondsToMilliseconds); |
There was a problem hiding this comment.
Could this result in UB if seconds < 0 or > UINT32_MAX?
Schema validator should take care about negative input, but static analysis or compiler warnings may not know about this. However we don't have cap on max timeout in schema.
Maybe we can log warning if value is too small or too big and apply some boundary values
| Config::Builder builder; | ||
|
|
||
| // initial_run_target is a required field, guaranteed non-null by the schema and verifier. | ||
| builder.setInitialRunTarget(config->initial_run_target()->str()); |
There was a problem hiding this comment.
I have mixed feelings about defensive coding, but if somebody prepares crazy config (bypassing verifier), we will crash instead of printing error message and exiting.
However if somebody is doing this, then maybe, they should not expect much support from launch manager.

Note: The code is not actually in use yet. The python script which does the translation from the user-facing json configuration to the flatbuffer json format is not part of the PR and will be added in a follow up PR.
Furthermore, the launch manager code needs to be adapted to use the new structure.
Background:
Relates To: #209