-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Added code to generate input data contract json #6472
Added code to generate input data contract json #6472
Conversation
Tasks/VsTest/distributedtest.ts
Outdated
inputDataContract['ExecutionSettings']['OverridenParameters'] = this.dtaTestConfig.overrideTestrunParameters; | ||
|
||
|
||
inputDataContract['AccessToken'] = this.dtaTestConfig.dtaEnvironment.patToken; // TODO: shrer: remove this and pass via ENV var and add login in the agent to read from both with preference to input file |
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.
pick this up on priority
Tasks/VsTest/distributedtest.ts
Outdated
//Modify settings file to enable configurations and data collectors. | ||
let settingsFile = this.dtaTestConfig.settingsFile; | ||
try { | ||
settingsFile = await settingsHelper.updateSettingsFileAsRequired |
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.
we should remove this call to update settings file now. Please run your L2s with these changes next
Tasks/VsTest/distributedtest.ts
Outdated
@@ -156,6 +273,26 @@ export class DistributedTest { | |||
return code; | |||
} | |||
|
|||
public static removeEmptyNodes(obj: any) { |
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.
can we write a test for this utility method. Also can we move this to the utility script file
Tasks/VsTest/distributedtest.ts
Outdated
|
||
tl.debug('Total env vars set is ' + Object.keys(envVars).length); | ||
// Pass the acess token as an environment variable for security purposes | ||
let envVars: { [key: string]: string; } = <{ [key: string]: string; }>{}; |
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.
node 5?
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.
yes it works. the ci passed
utils.Helper.printMultiLineLog(settings, (logLine) => { console.log('##vso[task.debug]' + logLine); }); | ||
}); | ||
} | ||
|
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.
i will not be accepting to enabling automatic rerun/tia getting invoked because certain properties/nodes exist in json. Customer should be clear on what he/she is enabling/disabling. it should be similar to task UI. if we develop simple UI tool for generation of the same, it should be on par with CI/CD UI experience.
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.
@cltshivash, @prawalagarwal, @acesiddhu whats our final call on this?
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.
This was discussed offline and we will go with this for now
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.
We have decided to close this review with the existing design and take this as feedback for the next phase/PR. This design should be closed before the next PR.
|
||
// Settings override inputs | ||
inputDataContract['ExecutionSettings']['TiaSettings']['Enabled'] = this.dtaTestConfig.tiaConfig.tiaEnabled; | ||
inputDataContract['ExecutionSettings']['TiaSettings']['RebaseLimit'] = this.dtaTestConfig.tiaConfig.tiaRebaseLimit; |
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.
model or prefer sections. currently settings are modified randomly. each section seperated by function might be clear
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.
currently the ordering is the same as was previously in the task to ensure there was a one to one mapping, will certainly do this
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.
This will be picked in the next phase when I add a model class. And will consider removing the remove empty nodes function.
// Invoke DtaExecutionHost with the input json file | ||
const inputFilePath = path.join(tl.getVariable('temp'), 'input.json'); | ||
DistributedTest.removeEmptyNodes(inputDataContract); | ||
writeFileSync(inputFilePath, JSON.stringify(inputDataContract)); |
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.
file exceptions
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.
actually no specific action is required here, there is a higher level catch block that prints out the error. And i checked the other file helper functions we have they just chain back the error and don't handle them in any way. Even if i did catch the only course of action is to fail the task and print error message which is done by the already enclosing catch block for this.
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.
Let us track the open points as tasks for our next phase
This reverts commit 0b28177.
* Revert "Users/shreyas r msft/input data contract parity tool test (#6676)" This reverts commit 19587af. * Revert "Added a model object for the input dataContract (#6652)" This reverts commit bca4241. * Revert "bug bash fixes (#6635)" This reverts commit 0550278. * Revert "Added code to generate input data contract json (#6472)" This reverts commit 0b28177. * Bumped version
No description provided.