Credentials vs Secure - options.js issue?

[wuhkuh]

Once again, good evening!

I have found a possible bug in the modernization from legacy in options.js. If I input 'secure' in my MoscaSettings, the whole thing works. If you replace secure with credentials, with the same entries - the whole thing stops working, even though the docs/source code mention credentials as the way to go.

The issue is here:

// translate mqtts options
var mqtts_enabled = !legacy.onlyHttp && legacy.secure; // it forgets to check legacy.credentials as a possible entry
if (mqtts_enabled) {
  var mqtts_interface = { type: 'mqtts' };

  if (legacy.secure.hasOwnProperty('port')) {
    mqtts_interface.port = legacy.secure.port;
  }

  modernized.interfaces.push(mqtts_interface);
}

I can upload a PR with a fix as early as tomorrow, but if you want to do this yourself that'd be fine.

Thanks!

[mcollina]

Pleasee send a PR, thanks!

[wuhkuh]

Hey @mcollina, After looking at the entire source code of options.js I came up with some questions.

• Why do we still use legacy code? (in v2) If we deprecate the legacy part there is less maintaining to do. It should also increase performance due to shorter scripts, even though that might only be at the initial setup of a TLS server.
• Why should some properties be copied carefully? The current implementation works fine - we could add the options to the conserved table as far as I know.
• Why don't we test for the 'secure' object inside the passed options before calling modernize? This would be an indicator of legacy code. This could increase performance when using modern code if implemented correctly.

The PR I will upload soon will probably contain some refactoring, and I can optimize the code further with answers to the previous questions.

Thanks!

[mcollina]

@wuhkuh mainly because I was very busy at the time, and I merged something that I should probably have not.

Why do we still use legacy code? (in v2) If we deprecate the legacy part there is less maintaining to do. It should also increase performance due to shorter scripts, even though that might only be at the initial setup of a TLS server.

A big PR that removes the legacy bits would be really welcomed. I'm not afraid of bumping the major and removing all that stuff.

Why should some properties be copied carefully?

No idea.

The current implementation works fine - we could add the options to the conserved table as far as I know. Why don't we test for the 'secure' object inside the passed options before calling modernize? This would be an indicator of legacy code. This could increase performance when using modern code if implemented correctly.

All that is not in a hot path, and I am not concerned about performance there. Maintainability is the first requirement.

Feel free to clean it all up and send a PR!

[wuhkuh]

@mcollina

I have forked the repository and changed the necessary files. It works on my own system, but before I add a PR I want to do some more testing. Do we have a protocol in place?

I have changed the README.md to suit the newest version, but the wiki still needs some updating.

[mcollina]

@wuhkuh no we don't. The test suite is really comprehensive. Check that the CLI still works. You will need to also update (and test) the examples.

[mcollina]

@wuhkuh would you like to update me with this one?

[wuhkuh]

@mcollina I totally forgot about this! I'll try to finish this in the short term.

[wuhkuh]

@mcollina I'm having some issues running tests on my machine, EBUSY errors using levelup in particular. Might have something to do with my A/V suite, but can't pinpoint the exact problem. In the meantime I've created a separate repo where I'll be running the tests using Travis.

I'll do some refactoring as well as installing standard, now that I have the time to do so. This might take a little longer, though. Is there anything else with high priority that needs attention?

[mcollina]

@wuhkuh not really, there is #589 if you want to embed that as well (it might help with testing).