feat(bucket): add enableLogging method #876
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #876 +/- ##
==========================================
+ Coverage 95.31% 95.36% +0.05%
==========================================
Files 11 11
Lines 1217 1231 +14
Branches 304 307 +3
==========================================
+ Hits 1160 1174 +14
Misses 29 29
Partials 28 28
Continue to review full report at Codecov.
|
src/bucket.ts
Outdated
| logObjectPrefix: config.prefix, | ||
| }, | ||
| }); | ||
| callback!(null, setMetadataResponse); |
There was a problem hiding this comment.
I don't think we should execute the callback here, only because it is wrapped inside of the try block and if a user error would occur, we would end up calling their callback twice
There was a problem hiding this comment.
I’m sure you’re right, although I’m not following the scenario. Could you elaborate how that would happen?
There was a problem hiding this comment.
Sure
bucket.enableLogging({prefix: 'hithere'}, err => {
if (err.code === 404) {
console.log('oh no!');
}
});Using that example, if we pretend the user forgets to check that err actually exists when it doesn't, it might result in a TypeError being thrown. That would move us to the catch section where we would execute the callback again with that very same TypeError.
There was a problem hiding this comment.
Gotcha, thanks!
src/bucket.ts
Outdated
| * | ||
| * bucket.enableLogging(config, function(err, apiResponse) {}); | ||
| * | ||
| * @example <caption>If the callback is omitted, we'll return a Promise. |
There was a problem hiding this comment.
@callmehiphop Ran into the end of the line. How's this looking?
There was a problem hiding this comment.
I actually think you have to run over the end in this case. Pretty sure our docs aren't smart enough to know that the closing tag is part of the caption still.
src/bucket.ts
Outdated
| try { | ||
| await this.acl.add({ | ||
| entity: 'group-cloud-storage-analytics@google.com', | ||
| role: 'WRITER', |
|
|
||
| try { | ||
| const [policy] = await this.iam.getPolicy(); | ||
| policy.bindings.push({ |
There was a problem hiding this comment.
@frankyn is this the correct configuration?
There was a problem hiding this comment.
+1, it's the role with the least number of permissions following the least privileges model.
|
|
||
| try { | ||
| const [policy] = await this.iam.getPolicy(); | ||
| policy.bindings.push({ |
There was a problem hiding this comment.
+1, it's the role with the least number of permissions following the least privileges model.
src/bucket.ts
Outdated
| try { | ||
| const [policy] = await this.iam.getPolicy(); | ||
| policy.bindings.push({ | ||
| members: ['cloud-storage-analytics@google.com'], |
There was a problem hiding this comment.
Ah, I missed the system test failure, cloud-storage-analytics@google.com is a group.
You should do:
members: ['group:cloud-storage-analytics@google.com']
src/bucket.ts
Outdated
| const [policy] = await this.iam.getPolicy(); | ||
| policy.bindings.push({ | ||
| members: ['cloud-storage-analytics@google.com'], | ||
| role: 'roles/storage.objectAdmin', |
There was a problem hiding this comment.
Use roles/storage.objectCreator instead. GCS shouldn't need to overwrite/delete data.
stephenplusplus
force-pushed
the
spp--23
branch
from
2da2509
to
c5e89be
Compare
|
Should we worry about making codecov happy? |
stephenplusplus
force-pushed
the
spp--23
branch
from
b4aa621
to
2c0faa1
Compare
|
@AVaksman that's a good point. I don't think we should error, but I just made the check less verbose to just take whatever they give us. PTAL |
stephenplusplus
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
|
LGTM |
|
Thanks everyone! |
Fixes #23
Hello! This adds the
bucket.enableLogging()shortcut method as discussed in #23.