-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement pipeMode Parameter to EvalCommand #346
Changes from all commits
838c9a8
2c38d09
fdec0b3
bb94b94
444149c
b5576ff
cac8e58
3202da9
38039fd
f39b91c
cea77d1
e040476
48fd41a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,9 +14,11 @@ export enum EvaluatedConfigOrigin { | |
EmptyValue = 'EMPTY_VALUE', | ||
} | ||
|
||
export type EvalCommandPipeMode = 'include' | 'forward'; | ||
|
||
export type EvalCommandReturn = { | ||
[key: string]: { | ||
context: { store: string; set: string; schema: string; key: string; cfgu: Cfgu }; | ||
context: { store: string; set: string; schema: string; key: string; cfgu: Cfgu; pipeMode: EvalCommandPipeMode }; | ||
result: { origin: EvaluatedConfigOrigin; source: string; value: string }; | ||
}; | ||
}; | ||
|
@@ -28,6 +30,7 @@ export type EvalCommandParameters = { | |
configs?: { [key: string]: string }; | ||
pipe?: EvalCommandReturn; | ||
validate?: boolean; | ||
pipeMode?: EvalCommandPipeMode; | ||
}; | ||
|
||
export class EvalCommand extends Command<EvalCommandReturn> { | ||
|
@@ -224,6 +227,15 @@ export class EvalCommand extends Command<EvalCommandReturn> { | |
return resultWithTemplates; | ||
} | ||
|
||
private removeForwards(result: EvalCommandReturn): EvalCommandReturn { | ||
const { pipe } = this.parameters; | ||
if (!pipe) { | ||
return result; | ||
} | ||
console.log(result); | ||
return _.omitBy(result, (current) => pipe[current.context.key] && current.context.pipeMode === 'forward'); | ||
} | ||
|
||
private validateResult(result: EvalCommandReturn): void { | ||
const { validate = true } = this.parameters; | ||
|
||
|
@@ -265,7 +277,7 @@ export class EvalCommand extends Command<EvalCommandReturn> { | |
} | ||
|
||
async run() { | ||
const { store, set, schema } = this.parameters; | ||
const { store, set, schema, pipeMode = 'include' } = this.parameters; | ||
|
||
await store.init(); | ||
|
||
|
@@ -277,6 +289,7 @@ export class EvalCommand extends Command<EvalCommandReturn> { | |
schema: schema.name, | ||
key, | ||
cfgu, | ||
pipeMode, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a mistake that leads to a bug. Consider a scenario where the 2nd eval also forwards the same keys as the previous, this case will lead to all the keys being lost due to the logic inside of the removal method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good one 💯 |
||
}, | ||
result: { | ||
origin: EvaluatedConfigOrigin.EmptyValue, | ||
|
@@ -308,6 +321,8 @@ export class EvalCommand extends Command<EvalCommandReturn> { | |
...this.evalTemplates(result), | ||
}; | ||
|
||
result = this.removeForwards(result); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thats should be after validation as all Configs must be validated |
||
|
||
this.validateResult(result); | ||
|
||
return result; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like in the other PR, let's recycle the EvalCommand usages into a function to keep this file leaner There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what other PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please adjust the schema examples to be more generic, perhaps we should always do tests on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?