Skip to content

Conversation

@pfefferle
Copy link
Member

I had problems with travis and the verbose mode of rsync:

The job exceeded the maximum log length, and has been terminated.

I would suggest to remove the flag, or at least to hide it behind an env var.

@getsource
Copy link
Member

I like the idea of having an environment variable to toggle it. Maybe something referencing verbose, or perhaps enabling "debug"?

@pfefferle
Copy link
Member Author

How should I call the var?

  • $WPT_VERBOSE
  • $WPT_VERBOSE_LOGGING
  • $WPT_VERBOSE_OUTPUT
  • ...

any suggestions?

@pfefferle
Copy link
Member Author

I made some changes... It is easier, to discuss about code ;)

@getsource
Copy link
Member

Thanks much! What do you think about WPT_DEBUG?

I'll leave a couple comments in the PR as well on specifics.

@getsource
Copy link
Member

Yes, it's working for me. I may be missing something, so apologies if so!
I think this is a bit more consistent with the way the rest of the string variables are handled in code so far.

If it doesn't exist at all (not in .env at all), it will end up false (bool).
if it is defined in .env as empty, it ends up as an empty string,

In both cases it's "set" and "empty".

@getsource
Copy link
Member

I'm seeing the 👍 but didn't want to assume incorrectly -- if you'd like to merge, go ahead -- if not, either I can, or of course we can keep discussing.

It looks good to me -- thank you!

@pfefferle
Copy link
Member Author

I like your change and I am fine with merging it 😊

@pfefferle pfefferle requested a review from getsource April 24, 2020 10:12
@pfefferle
Copy link
Member Author

I am the MR owner, so I cannot approve the changes.

Copy link
Member

@getsource getsource left a comment

Choose a reason for hiding this comment

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

Looks good to me -- thank you!

@pfefferle pfefferle merged commit 190d3e1 into master Apr 24, 2020
@pfefferle pfefferle deleted the reduce-load branch April 24, 2020 10:27
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.

3 participants