fix: add warning for unsupported keepAcl param in file#copy

[AVaksman]

Related to #836

• Tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

As per #839 (comment)

[stephenplusplus]

This is scary to me, because we've always considered our chance to communicate dev-only messages is pre-installation via the changelog, but not at runtime. That could potentially interrupt their users, since we are commonly a base layer in more complex applications.

[stephenplusplus]

cc @bcoe @callmehiphop for thoughts!

[callmehiphop]

I might be misremembering, but I seem to recall using the console to warn users about deprecated methods in the auth client as well (paging @JustinBeckwith). I'm ok with this, but think we should take a hard stance of how we communicate this kind of change across the board.

[bcoe]

I've been known to use this approach in yargs:

https://github.com/yargs/yargs/blob/5d7ad989a851398587a0349cdd15344769b4cd79/yargs.js#L1035

And haven't received many complaints; my general thinking has been.

• it's less disruptive to start by warning folks than throwing.
• it draws attention to a change that will eventually be breaking.
• Node itself has done this for a lot of features, e.g., Buffer, so the community is accustomed to it.

In this case, my gut was that only a small percentage of folks would be explicitly setting keepAcl so it's probably fairly low impact.

[JustinBeckwith]

👋 I have opinions here!
https://github.com/googleapis/google-auth-library-nodejs/blob/master/src/messages.ts

A few thoughts:

• We should only emit these warnings once per process invocation
• Please use process.emitWarning instead of console.warn as it plays nicely with the cmd flags of nodejs. You can learn more here: https://medium.com/@jasnell/introducing-process-warnings-in-node-v6-3096700537ee
• I like including error codes with these to make it easier for people to search for

As with all things, final say here is deferred to @bcoe

[bcoe]

Seems like the right move is to switch to process.emitWarning 👍 and let's land this.

For this specific case, I don't have a strong opinion about including an error code; seems smart in the case of an HTTP response or a file operation going off the rails, but not quite sure what a good option would be for this.

[AVaksman]

Done, thanks
@JustinBeckwith could I trouble you to look over the switch to process.emitWarning once per process (last commit).

[codecov]

Codecov Report

Merging #841 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #841      +/-   ##
==========================================
+ Coverage   95.22%   95.25%   +0.02%     
==========================================
  Files          11       11              
  Lines        1194     1201       +7     
  Branches      296      298       +2     
==========================================
+ Hits         1137     1144       +7     
  Misses         29       29              
  Partials       28       28

Impacted Files	Coverage Δ
src/file.ts	98.15% <100%> (+0.02%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27fa02f...ad6b919. Read the comment docs.

[bcoe]

let's switch warn to process.emitWarning as-per @JustinBeckwith's suggestion.

[JustinBeckwith]

LGTM - only ask is that we land this as a fix since it has a material change to the behavior of the API