MM-13027: Don't include a diagnostics segment key in dev builds. #2131
Conversation
grundleborg
requested review from
lieut-data,
mkraft and
jespino
and removed request for
lieut-data
| @@ -146,7 +146,7 @@ export default class Root extends React.Component { | |||
| const diagnosticId = this.props.diagnosticId; | |||
|
|
|||
| /*eslint-disable */ | |||
| if (segmentKey != null && segmentKey !== '' && this.props.diagnosticsEnabled) { | |||
| if (segmentKey != null && segmentKey !== '' && !segmentKey.startsWith('placeholder') && this.props.diagnosticsEnabled) { | |||
There was a problem hiding this comment.
I don't understand the purpose of this code.
The segmentKey is obtained from a constant, so, It'll never be null, and maybe, It'll be an empty string (if somebody modify the code to "disable" segment removing the key), but then, the placeholder approach is "redundant" and a bit confusing for me. For me is easier to set the constant to an empty string (so the purpose is clear). Anyway, it is a constant, so we can remove the entire block, or move it to a config setting in the server.
I'm ok on merging this as is, because I understand the idea behind, but I think is a confusing way to write it.
There was a problem hiding this comment.
It's done this way because Jenkins runs a sed s/placeholder_segment_key/$real_segment_key/g over the codebase, to update that constant to an appropriate value depending on the build type.
There was a problem hiding this comment.
Worth an inline command as such?
grundleborg
force-pushed
the
MM-13027
branch
from
9c0c311
to
0e5031a
Compare
grundleborg
added
4: Reviews Complete
grundleborg
removed
the
4: Reviews Complete
amyblais
added
Changelog/Not Needed
amyblais
added
Changelog/Not Needed
Summary
Don't include a diagnostics segment key in dev builds.
Ticket Link
https://mattermost.atlassian.net/browse/MM-13027