Skip to content
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

companion: set debug based on NODE_ENV only if the env var is available #2189

Merged
merged 1 commit into from
Apr 13, 2020

Conversation

ifedapoolarewaju
Copy link
Contributor

in a system where the NODE_ENV variable is never used as part of their config variables to begin with, it'll be unsafe to assume the system is in debug mode by merely checking NODE_ENV !== 'production'

@goto-bus-stop
Copy link
Contributor

Hm, should we use === 'development' then?

@ifedapoolarewaju
Copy link
Contributor Author

didn't want to go with that because of === 'test', and the likes

Copy link
Contributor

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough :) happy with this then!

@arturi
Copy link
Contributor

arturi commented Apr 10, 2020

I also wonder if we need to be explicit that we want to expose debug information, otherwise something like staging (relatively publicly available) could also leak something?

@kvz
Copy link
Member

kvz commented Apr 10, 2020

In addition we should replace actual credentials with:

  • ********, or:
  • ${CREDENTIAL_FOO} (obfuscated for security)

in our lowest level logging function (so right before it hits stdout/stderr)

@ifedapoolarewaju
Copy link
Contributor Author

In addition we should replace actual credentials with:

  • ********, or:
  • ${CREDENTIAL_FOO} (obfuscated for security)

in our lowest level logging function (so right before it hits stdout/stderr)

@kvz do you think it's fine if I make this change in a separate PR? I'm suggesting this because I think it my be subject to a longer review process.

@kvz
Copy link
Member

kvz commented Apr 13, 2020

Yes 👌

@ifedapoolarewaju ifedapoolarewaju merged commit 3304358 into master Apr 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the companion-node-env branch April 13, 2020 22:21
@ifedapoolarewaju
Copy link
Contributor Author

I also wonder if we need to be explicit that we want to expose debug information, otherwise something like staging (relatively publicly available) could also leak something?

@arturi so you mean to explicitly mention this in the docs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants