Task/294 task run checkproperties on config load #324

DougMidgley · 2022-05-19T21:47:33Z

PR details

What is the purpose of this pull request?

Other, please explain: Internal

What changes did you make? (Give an overview)

rework loading parameters logic into a new class to help reduce circular dependencies and remove bloat in common classes

Is there anything you'd like reviewers to focus on?

initial stab - feedback requested more generally about approach

Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
ESLint & Prettier are not outputting errors or warnings
README.md updated (if applicable)
CHANGELOG.md updated
ran npm run docs to update developer docs

JoernBerkefeld · 2022-05-20T21:14:40Z

Wrong target? ;)

…into task/294-task-run-checkproperties-on-config-load # Conflicts: # lib/util/file.js # lib/util/util.js

JoernBerkefeld

good idea, needs a few tests & tweaks though

lib/util/cli.js

JoernBerkefeld · 2022-06-13T12:52:37Z

lib/index.js

- properties = properties || File.loadConfigFile();
- if (!(await Util.checkProperties(properties))) {
- return null;
- }


the return null case is no longer executed. If the config is missing or bad this method would continue (and then eventually fail).

With a few tweaks your new Config.getProperties could at least throw an error or return null. It's calling checkProperties internally but not testing its return value.

would it work to use throw error in checkProperties, can also add missing fields as an error property for that use that. I think that would clean up a couple of things

JoernBerkefeld · 2022-06-13T12:55:17Z

lib/index.js

- if (!(await Util.checkProperties(properties))) {
- return null;
- }


the return null case is no longer executed. If the config is missing or bad this method would continue (and then eventually fail).

With a few tweaks your new Config.getProperties could at least throw an error or return null. It's calling checkProperties internally but not testing its return value.

lib/util/config.js

lib/index.js

lib/util/cli.js

JoernBerkefeld · 2022-06-13T13:02:03Z

lib/util/cli.js

@@ -85,7 +86,7 @@ const Cli = {
 */
 async getCredentialObject(properties, target, isCredentialOnly, allowAll) {
 try {
- if (!(await Util.checkProperties(properties))) {
+ if (!(await Config.checkProperties(properties))) {


does this logic still work, given that you run checkProperties inside of getProperties already?

I assume you mean, is it needed? I would say actually no, so we can throw errors earlier on getProperties?

…into task/294-task-run-checkproperties-on-config-load # Conflicts: # docs/dist/documentation.md # lib/cli.js # lib/index.js # lib/util/businessUnit.js # lib/util/file.js # lib/util/init.js # lib/util/util.js

JoernBerkefeld

lots still to do here. I'd ask that we please follow through with my PRs first and once this reaches production readiness I'd happily adjust to the new reality.

our tool has to work properly and have reasonably nice error output for users in a CLI. Therefore, we cannot just throw errors without handling them. Prefixing our errors with "Retrieve failed: ...", "Document failed: ...." is also not very user friendly. For one, the user just executed that command. whatelse should have failed? Therefore, whatever error message comes needs to be printable as is - and I've gone through great lengths to make that happen over the past months.

The few times you added try-catch blocks seem like a step back in that regard. For the many other lines on which you had to replace getting the config, error handling is just missing and would result in ugly stack traces being presented to our already not-so-CLI-friendly user base.

To get this ready for merge you'd have to either go back on your change concerning the return value of the config getter - a simple refactoring would have also done us a favor without causing the need to retest every single method just because of this PR alone - or you consistently handle potential errors caused by your new code gracefully in either lib/index.js or in lib/cli.js.

please note the 11+ hidden conversations

JoernBerkefeld · 2022-06-27T22:33:35Z

lib/util/config.js

+ if (silent) {
+ const err = new Error('Missing Fields');
+ err.missingFields = missingFields;
+ throw err;


how is this "silent"?

silent was clearly marked as the mode we use to process the results outside of this method - after returning an array of strings (which you still have in the jsdoc for this method)

so, i think we need to improve consistency on "silent". Currently I can see some cases where it means no CLI messages and others where it means no interaction (no inquirer commands). In particular, for this case, the one location that uses missingFields has a try/catch block around to pick these up correctly.
Additional point, I might wanna do is merge/split differently the checkProps and getProps methods to ensure checks contains ALL the checks and get is just the high level loading -> thoughts?

this feels like a "lets sit at the same desk and talk" topic

went through the code again but cant help but think that wrapping this in an error adds no value here. It does add extra lines of code though and complexity. we can argue about renaming the parameter "silent" to "returnList" or sth like it but that's really the expected outcome here and while it's true that you could do that with try-catch and attaching it to the error, there are probably also 20 other ways but none are better than simply returning the array that the other method is looking for here.

JoernBerkefeld · 2022-06-27T22:54:37Z

lib/util/config.js

+ if (responses.runUpgradeNow) {
+ // use _execSync here to avoid a circular dependency
+ this.execSync('mcdev', ['upgrade']);
+ }
+ throw new Error(errorMsgOutput);


why should we print errors after running mcdev upgrade?

previously this returned false - which would fail the check. Basically took this over - it probably should just be removed now, agree?

after we've run mcdev we should end execution. we COULD continue automatically but ensuring that this runs smoothly then might just not be worth the effort

lib/index.js

lib/Builder.js

lib/Deployer.js

…into task/294-task-run-checkproperties-on-config-load # Conflicts: # docs/dist/documentation.md # lib/Deployer.js # lib/cli.js

…into task/294-task-run-checkproperties-on-config-load

…into task/294-task-run-checkproperties-on-config-load # Conflicts: # docs/dist/documentation.md

JoernBerkefeld · 2022-06-30T16:49:37Z

lib/cli.js

@@ -374,6 +374,10 @@ yargs
 })
 .demandCommand(1, 'Please enter a valid command')
 .strict()
+ .fail((_, err) => {
+ Util.logger.error(err.message);
+ process.exit(1);


our adaption of Util.logger.error already sets the exit code to 1

sfmc-devtools/lib/util/util.js

Line 499 in bc01163

error: function (msg) {

also there is a method to wrap around exit code 1:

sfmc-devtools/lib/util/util.js

Line 131 in bc01163

signalFatalError() {

apparently the right way to do it for async-functions was process.exitCode = 1;
here more on the why: https://nodejs.dev/learn/how-to-exit-from-a-nodejs-program

JoernBerkefeld · 2022-06-30T16:55:16Z

lib/index.js

+ if (type && !MetadataTypeInfo[type]) {
+ Util.logger.error(`:: '${type}' is not a valid metadata type`);
+ return;
+ }


should be done before getting the config to increase speed of execution.
also, given that this is highest level execution - should we not switch the error log into a throw error instead?

JoernBerkefeld · 2022-06-30T16:56:39Z

lib/index.js

+ Util.logger.error('mcdev.document ' + ex.message);
+ Util.logger.debug(ex.stack);
+ Util.logger.info(
+ 'If the directoy does not exist, you may need to retrieve this BU first.'
+ );


maybe throw error instead given the new catch in cli.js?

JoernBerkefeld · 2022-07-01T14:10:14Z

lib/util/config.js

+ * @param {boolean} [silent] omit throwing errors and print messages; assuming not silent if not set
+ * @returns {object} central properties object
+ */
+ async getProperties(silent) {


note - this is how we define methods in other util classes

JoernBerkefeld · 2022-07-01T14:10:31Z

lib/util/config.js

+ * @param {boolean} [silent] set to true for internal use w/o cli output
+ * @returns {Promise.<boolean | string[]>} file structure ok OR list of fields to be fixed
+ */
+ checkProperties: async function (properties, silent) {


here you switch to a different notation. any reason?

JoernBerkefeld · 2022-07-01T14:11:23Z

lib/util/config.js

+ *
+ * @returns {object} default properties
+ */
+ getDefaultProperties: async function () {


and again the other type of notation.
this sets a property to an anonymous method rather than define the method getDefaultProperties as part of the Config object

https://stackoverflow.com/a/336868/818732

DougMidgley · 2022-07-03T08:48:24Z

lib/util/config.js

 * @returns {object} central properties object
 */
- async getProperties(silent) {
+ async getProperties(skipChecks = false) {


again, i would say withChecks = true here to avoid double negatives. https://blog.devgenius.io/code-smell-51-double-negatives-993b6160f3e1

JoernBerkefeld · 2022-07-04T13:53:41Z

moving this out of 4.0.0. it's a waste of time at this point.
suggesting to re-do the PR only with the purpose of reducing file system reads which is actually a great idea.

but without changing the error-recognition & reporting logic (exception vs boolean return)

DougMidgley requested a review from JoernBerkefeld May 19, 2022 21:47

DougMidgley linked an issue May 19, 2022 that may be closed by this pull request

[TASK] Run checkProperties on Config Load #294

Open

douglas.midgley added 2 commits May 22, 2022 22:24

#294: Initial Carve out

6522f65

Merge branch 'develop' of https://github.com/Accenture/sfmc-devtools …

3a39960

…into task/294-task-run-checkproperties-on-config-load # Conflicts: # lib/util/file.js # lib/util/util.js

DougMidgley changed the base branch from main to develop May 22, 2022 20:29

DougMidgley marked this pull request as ready for review May 22, 2022 20:30

JoernBerkefeld requested changes Jun 13, 2022

View reviewed changes

douglas.midgley added 3 commits June 19, 2022 11:52

#294: capitalization change

3e555d7

#294: moved to error-based handling

e92652b

Merge branch 'develop' of https://github.com/Accenture/sfmc-devtools …

e4c5377

…into task/294-task-run-checkproperties-on-config-load # Conflicts: # docs/dist/documentation.md # lib/cli.js # lib/index.js # lib/util/businessUnit.js # lib/util/file.js # lib/util/init.js # lib/util/util.js

JoernBerkefeld requested changes Jun 27, 2022

View reviewed changes

douglas.midgley added 5 commits June 28, 2022 22:15

#294: top level error handling on CLI

e669631

#294: review feedback

c71eb3f

Merge branch 'develop' of https://github.com/Accenture/sfmc-devtools …

350aecd

…into task/294-task-run-checkproperties-on-config-load # Conflicts: # docs/dist/documentation.md # lib/Deployer.js # lib/cli.js

Merge branch 'develop' of https://github.com/Accenture/sfmc-devtools …

1e93419

…into task/294-task-run-checkproperties-on-config-load

Merge branch 'develop' of https://github.com/Accenture/sfmc-devtools …

bc01163

…into task/294-task-run-checkproperties-on-config-load # Conflicts: # docs/dist/documentation.md

JoernBerkefeld reviewed Jun 30, 2022

View reviewed changes

JoernBerkefeld assigned DougMidgley Jul 1, 2022

JoernBerkefeld added the chore Jira issue-type "Task" label Jul 1, 2022

JoernBerkefeld added this to the 4.0.0 milestone Jul 1, 2022

JoernBerkefeld requested changes Jul 1, 2022

View reviewed changes

JoernBerkefeld added 5 commits July 1, 2022 16:45

#346: turn auth class into properly named (and resolved) object

bb47453

#294: jsdoc and function expression changed to function declaration

45e2cf1

https://stackoverflow.com/a/336868/818732

#294: fix bad reference

7ffd72e

#294: extracted getProblems()

4157213

#294: skipChecks param in getProperties()

0ba3e76

DougMidgley commented Jul 3, 2022

View reviewed changes

douglas.midgley and others added 4 commits July 3, 2022 23:28

#294: missing Util ref

169650e

#294: bad class reference fixed

cf78dcc

#294: fix error handler to no longer fail on "mcdev" without params

2c0f27f

#294: otherwise causing loop

2015df4

JoernBerkefeld modified the milestones: 4.0.0, 4.1.0 Jul 4, 2022

JoernBerkefeld marked this pull request as draft July 4, 2022 13:53

JoernBerkefeld modified the milestones: 4.2.0 , 4.3.0 Nov 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task/294 task run checkproperties on config load #324

Task/294 task run checkproperties on config load #324

DougMidgley commented May 19, 2022 •

edited by JoernBerkefeld

Loading

JoernBerkefeld commented May 20, 2022

JoernBerkefeld left a comment

JoernBerkefeld Jun 13, 2022

DougMidgley Jun 19, 2022

JoernBerkefeld Jun 13, 2022

JoernBerkefeld Jun 13, 2022

DougMidgley Jun 19, 2022

JoernBerkefeld left a comment •

edited

Loading

JoernBerkefeld Jun 27, 2022

DougMidgley Jun 28, 2022

JoernBerkefeld Jun 30, 2022

JoernBerkefeld Jul 1, 2022

JoernBerkefeld Jun 27, 2022

DougMidgley Jun 28, 2022

JoernBerkefeld Jun 30, 2022

JoernBerkefeld Jun 30, 2022

JoernBerkefeld Jun 30, 2022

JoernBerkefeld Jun 30, 2022 •

edited

Loading

JoernBerkefeld Jun 30, 2022

JoernBerkefeld Jun 30, 2022

JoernBerkefeld Jul 1, 2022

JoernBerkefeld Jul 1, 2022

JoernBerkefeld Jul 1, 2022

DougMidgley Jul 3, 2022

JoernBerkefeld commented Jul 4, 2022

Task/294 task run checkproperties on config load #324

Are you sure you want to change the base?

Task/294 task run checkproperties on config load #324

Conversation

DougMidgley commented May 19, 2022 • edited by JoernBerkefeld Loading

PR details

What is the purpose of this pull request?

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

Checklist

JoernBerkefeld commented May 20, 2022

JoernBerkefeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoernBerkefeld left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoernBerkefeld Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoernBerkefeld commented Jul 4, 2022

DougMidgley commented May 19, 2022 •

edited by JoernBerkefeld

Loading

JoernBerkefeld left a comment •

edited

Loading

JoernBerkefeld Jun 30, 2022 •

edited

Loading