HPCC-20458 Allow File Access keys to be defined in the environment #12027
Conversation
|
https://track.hpccsystems.com/browse/HPCC-20458 |
|
@jpmcmu Please review Please let me know if you have any questions. |
|
@kenrowland Maybe I'm wrong, but this line looks like very suspicious to me: especially this part: "this=this@entry=0x0" where "this=0x30" |
|
@AttilaVamos one important thing to note is that this all works on Ubuntu. It's also possible that my latest test has a problem |
|
@kenrowland I know. |
|
@kenrowland I should check something. Give me ~ half an hour. |
|
@AttilaVamos the failure on my last commit is a logic error. I did not test on Ubuntu, so consider it an invalid workaround. |
|
Well Done! (That's why we really need Smoketest and OBT.) |
|
@AttilaVamos I agree! That's why once I figured out how to implement unit tests, I started adding them for all my code. I still have more I will be adding as time goes by so that when I make a change to core code, it's easy to test it. One more commit coming to clean up. |
kenrowland
force-pushed
the
HPCC-20458
branch
2 times, most recently
from
50b6cf8
to
18247a1
Compare
| std::string inputName(input.FindMember("name")->value.GetString()); | ||
| std::string type(input.FindMember("type")->value.GetString()); | ||
| pInput = inputValueFactory(type, inputName); | ||
| std::string varName(varValue.FindMember("name")->value.GetString()); |
There was a problem hiding this comment.
Do the "name" and "type" members always exist?
There was a problem hiding this comment.
Yes, they are required by the schema.
| // Get the count (optional, default is 1) | ||
| it = dataObj.FindMember("count"); | ||
| if (it != dataObj.MemberEnd()) | ||
| pOp->m_count = trim(it->value.GetString()); |
There was a problem hiding this comment.
It looks like m_count is a string at this point. Would it be better to convert it to an int here to catch invalid number strings early?
There was a problem hiding this comment.
No because they could be substitution variables in the form {{varname}}. For that reason, they are kept in their raw for until used whereby a substitution and conversion to a number is done. At that time an exception, during execution of the template, can be thrown if there is a problem.
| // Get the starting index (optional, for windowing into a range of values) | ||
| it = dataObj.FindMember("start_index"); | ||
| if (it != dataObj.MemberEnd()) | ||
| pOp->m_startIndex = trim(it->value.GetString()); |
There was a problem hiding this comment.
Same comment as above for m_startIndex.
There was a problem hiding this comment.
See comment for m_count above.
| rapidjson::Value::ConstMemberIterator saveInfoIt = dataObj.FindMember("save_nodeid"); | ||
| if (saveInfoIt != dataObj.MemberEnd()) | ||
| { | ||
| pOp->m_saveNodeIdName = saveInfoIt->value.GetObject().FindMember("save_name")->value.GetString(); |
There was a problem hiding this comment.
Is "save_name" guaranteed to exist?
There was a problem hiding this comment.
Yes, it is required by the schema if the save object exists.
| saveInfoIt = attr.FindMember("save_value"); | ||
| if (saveInfoIt != attr.MemberEnd()) | ||
| { | ||
| newAttribute.saveValue = saveInfoIt->value.GetObject().FindMember("save_name")->value.GetString(); |
There was a problem hiding this comment.
Same question from above is "save_name" always present?
There was a problem hiding this comment.
The same sub schema definition is use here as well.
| if (!attr.saveValue.empty()) | ||
| { | ||
| bool found=false, set=false; | ||
| std::shared_ptr<Variable> pInput = pInputs->getVariable(attr.saveValue); |
There was a problem hiding this comment.
Can pInput be a nullptr here?
There was a problem hiding this comment.
No. Previously the attr.saveValue member would have been used to add an entry by that name to the Variables object. However, the getVariable method will throw an exception if the requested variable does not exist.
| } | ||
| else | ||
| { | ||
| // if (!m_values.empty()) |
There was a problem hiding this comment.
Any reason to keep the commented out code?
There was a problem hiding this comment.
Um, no. I thought I had removed all the commented code, guess I missed this one. I will remove.
Add new CLI envmod for processing modification templates to modify environment files. Update modification template support. Add unit tests for new features. Signed-off-by: Ken Rowland <kenneth.rowland@lexisnexisrisk.com>
kenrowland
force-pushed
the
HPCC-20458
branch
from
18247a1
to
87af079
Compare
|
Automated Smoketest: ✅ Unit tests result:
Regression test result:
HPCC Stop: OK
|
|
@kenrowland looks good |
|
@richardkchapman Please merge |
Add new CLI envmod for processing modification templates to modify
environment files.
Update modification template support.
Add unit tests for new features.
Signed-off-by: Ken Rowland kenneth.rowland@lexisnexisrisk.com
Refactoring and updates
Type of change:
Checklist:
Testing: