Forum Discussion

Stuart_Weenig's avatar
Icon for Mastermind rankMastermind
2 years ago

Wait, why is this even a change?

If the ME team needs me to write a linter so that stupid updates like this don’t go out, I charge $1M/hour.

Seriously? `/r/n` vs `/n`?

7 Replies

  • Repo calls the current installed version an “unpublished propertysource”.

    There’s no copyright info in the script, but I’m sure I didn’t write it.

  • From version “Installed” to version “1.0.0”. I suspect the stitching between existing modules and modules in core really doesn’t work at all.

  • mray's avatar
    Icon for LM Conqueror rankLM Conqueror

    @Stuart Weenig Are you able to see the version info? What version are you updating from/to?

  • Because Vin didn’t know, I’ll elaborate:

    \r\n is a Windows style carriage return. Those two characters together are known as a carriage return and newline. Comes from when typewriters had two instructions to move to the new line: return the carriage to the beginning of the line and advance the roller one line. 

    \n is a Unix style carriage return, because Unix moved beyond typewriters back in the ‘70s and Microsoft didn’t.

    Today they are practically equivalent. Most modern text editors will give you the option of using one or the other, despite your system’s OS. Many will detect what is already in use in a text file and will continue to use they. Most also give you a one click option to switch from one to the other a la replace(“\r\n”,”\n”). 

    The problem is that some programming language interpreters on Linux do not do well when presented with \r\n at the end of the line (especially the shebang line). It doesn’t treat \r\n as a single thing and instead reaches \r and doesn’t know what to do with it. It’s not a command or other kind of directive.

    Windows systems do not have this problem because they properly interpret both \r\n and \n as equivalent. 

    For compatibility across all Collector platforms, none of the scripts should use \r\n and should use \n. I know LM knows this because in the same batch where this update was, there was a script that replaced all the \r\n’s with \n’s. 

    They also already have a linter for modules that checks all kinds of things. I wonder if they aren’t using it any more.

  • mray's avatar
    Icon for LM Conqueror rankLM Conqueror

    @Stuart Weenig I started a GoFundMe for your fee. I would love to see this implemented 👍

  • Re: Microsoft_SQLServer_ConnectionStatus: yeah, we override that threshold at /Devices by Type with “> 0 1 1” for just that reason.

     Check the spoiler to see how/why we do this.

    It would be nice if there was a separate severity for things like that. Something that needs attention but doesn’t positively indicate an outage in the infrastructure. We use warning for that, but that has its drawbacks.

  • Speaking of “lack of QA”, avoid loading the new Microsoft_SQLServer_ConnectionStatus module. Whoever wrote that one decided that lack of a connection string should generate a critical alert, definitely far from how modules typically work with lack of expected properties. I opened a bug ticket on it, no idea if/when they plan to fix.  Maybe they are using ChatPGT as developers now.