luigi
luigi copied to clipboard
spotify
Add ExternalProgramTask param debug_environment
Description
This PR adds a parameter named debug_environment to the class ExternalProgramTask. If it is set to False, environment variables won't be printed to stderr when an error occurs.
Motivation and Context
There are several scenarios in which you would not like to see all your environment variables being printed onto the stderr stream. For example, it is possible that they include private access tokens. It should be possible to run your pipeline on Travis CI using Shippable or something similar without revealing this information to the public.
Have you tested this? If so, how?
I have included unit tests, but I still need to check whether Travis will accept them. :D
These changes would also be really useful for me. I would love to see this merged soon, if possible. @honnix @Tarrasch @dlstadther
I am a bit confused that none of the travis logs contains the string debug_environment. Are the added tests actually run anywhere?
@Tarrasch Thanks for the review! print_cmdline_on_failures is not actually what this PR allows to change. It only allows to disable printing of environment variables. So what's about print_environment_on_failures? (But that's a quite long name ...)
@LinqLover , sorry I only look through the PRs on a very infrequent basis these days. Sorry!
That said, I still don't get the name debug_environment. It doesn't sound like it is a bool to begin with. Further, the Parameter description says something about "debug variables", afaik "debug" isn't a verb that means "print to screen". I understand one uses the verb like that but it's not universal among programmers at all.
@Tarrasch Okay, would you agree with the name print_environment_on_failures? :)
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If closed, you may revisit when your time allows and reopen! Thank you for your contributions.
![stale[bot] avatar](https://avatars.githubusercontent.com/in/1724?v=4 "Published by a pub.dev verified publisher"){kind=small}
@Tarrasch This PR has been ready for a long time and is only lacking properly naming. I would like to finally merge it, how would you like the parameter to be named?
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If closed, you may revisit when your time allows and reopen! Thank you for your contributions.