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

Add option to output raw csp (fix #81) #84

Merged

Conversation

YoranBrondsema
Copy link
Contributor

  1. Fixes Difficult to extract a usable CSP using csp-headers command #81. With a --raw option, the configuration guidelines are omitted.
  2. Output to the standard output rather than to error. Maybe there's a good reason for sending it to error. In that case this change can be reverted. But otherwise, it seems semantically wrong to do that.
  3. Add a small description for the report-uri option.

@jelhan
Copy link
Collaborator

jelhan commented Feb 25, 2019

Did you considered --silent flag? ember test is already supporting that one.

In contrast to --quiet flag --silent is often used as the opposite of --verbose: do only print minimal data user has asked for. An example is curl:

   -s, --silent
         Silent  or  quiet  mode.  Don't  show progress meter or error messages.
         Makes Curl mute. It will still output the data you ask for, potentially even
         to the terminal/stdout unless you redirect it.

@YoranBrondsema
Copy link
Contributor Author

Thanks for your suggestion @jelhan. The semantics of --silent indeed correspond better.

@rwjblue: is it possible to get your feedback so that we can get this PR merged?

@sandstrom
Copy link
Collaborator

sandstrom commented Mar 29, 2019

@YoranBrondsema Looks good!

However, I'm hesitant to merge due to the stderr -> stdout switch (3c8b3bb). While I think the change makes sense, maybe there is a reason for it that I don't know about.

So I'd like to check with @rwjblue to see that there aren't a special reason for outputting to stderr.

@YoranBrondsema YoranBrondsema force-pushed the add-option-to-output-raw-csp branch from 3c8b3bb to 5c2bcca Compare March 31, 2019 15:39
@rwjblue
Copy link
Member

rwjblue commented Apr 1, 2019

@sandstrom is correct, emitting to stdout breaks things that attempt to parse the console output for test pass/fail. However, it may not be a factor in our case (e.g. if we only emit during development but not tests), right?

@YoranBrondsema - I'm also curious what motivated that particular change?

@YoranBrondsema
Copy link
Contributor Author

@rwjblue OK I understand the reasoning behind sending to stderr. The motivation to send to stdout is that

  1. I was surprised that I couldn't read the output when consuming by a Ruby script. Only when I dug into the code did I see that it was sending to stderr, so I had to adapt the script accordingly.
  2. It doesn't "feel" right as we're not outputting some error message, we're just displaying some configuration information. So semantically I think stdout makes more sense than stderr.

Either way, I realize now that this change does not belong in this PR. So I will undo it, so that this PR only adds the --silent flag.

@YoranBrondsema YoranBrondsema force-pushed the add-option-to-output-raw-csp branch 3 times, most recently from eba9a48 to c515ca4 Compare April 3, 2019 19:42
@sandstrom
Copy link
Collaborator

sandstrom commented Apr 4, 2019

@YoranBrondsema I spoke with rwjblue about this. I interpreted his reasoning like this:

For things that are consumed by tests, we cannot use stdout since it will mess with the pass/fail check. However, since this command won't be used during tests, it's fine to switch.

So, in other words, feel free to open a new PR with the switch from stderr -> stdout. If you can, write a line or two about whether that would reasonably affect any tests and ping rwjblue in that PR.


Happy to merge this PR! But could you look at the Travis issue first and see if you can fix that (perhaps just by forcing a rerun of the tests).

@YoranBrondsema YoranBrondsema force-pushed the add-option-to-output-raw-csp branch from c515ca4 to 1fc99a9 Compare April 7, 2019 17:41
@YoranBrondsema YoranBrondsema force-pushed the add-option-to-output-raw-csp branch from 1fc99a9 to 6a139d1 Compare April 7, 2019 18:57
@sandstrom
Copy link
Collaborator

@YoranBrondsema I don't know why one travis job fails, but I'm willing to merge anyway because I cannot see that anything changed in this PR could cause those errors.

Thanks for your work on this! 🏅

@sandstrom sandstrom merged commit 7c0110f into adopted-ember-addons:master Apr 10, 2019
@YoranBrondsema
Copy link
Contributor Author

I tried re-running it but it seems to consistently fail on the beta release of Ember.js. Thanks for the merge!

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

Successfully merging this pull request may close these issues.

Difficult to extract a usable CSP using csp-headers command
4 participants