Fix requester pays check#824

[lbergelson]

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass <- Normal tests and linter pass but I was unable to configure a service account correctly to pass the integration tests.
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #824 ☕️

• The error message received when querying for requester pays status changed to include an 'a' which caused strict matching to fail.
• This fixes and standardizes the error message to use the same string for both the code and the tests.

[lbergelson]

If someone knows where that error message is generated so we could refer to it directly it would be good....

[droazen]

@lbergelson As part of this PR, can you re-enable the requester pays tests disabled in #825?

[lbergelson]

@droazen That seems reasonable assuming they pass now.

[kshakir]

@lbergelson have you considered a simpler match string that may have a better chance of not breaking in the future?

For example, it looks like @breilly2, @jgainerdewar, and @mcovarr updated cromwell to use "requester pays bucket but no user project":
broadinstitute/cromwell#6689

Meanwhile terra-ui searches for the even more basic string "requester pays", and didn't even need to be updated with the last server-side change: https://github.com/DataBiosphere/terra-ui/blob/e34bdf0b81d7d2750787108ac06238787a363726/src/libs/ajax.js#L76

[sydney-munro]

We are working with the API team to provide a more permanent solution to this issue and with a fallback to checking the error message.

See: #841

[lbergelson]

@kshakir Yeah, I was going to reduce it to just "requester pays" but I figured the smaller the change the faster it might get merged. Those extra few characters are hard to review!

[lbergelson]

@sydney-munro Thank you, that's great. It's a very fragile mechanism at the moment. Even fixing it so that the storage service returns a better structured error message is still a bit awkward though. The biggest remaining issue is that the call requires storage.bucket.get permissions to work at all which is not always available ( and isn't necessary to read the file) just check requester pays status. It seems like it should be altered to not have to require that permission. Getting this fixed quickly though is the priority.

[droazen]

@sydney-munro Sounds good, but what's the timeline for merging the more permanent solution? If it's more than a week or so, could we possibly merge the patched error message check as a stopgap measure just to get this feature working again? Thanks!

[droazen]

@lbergelson Since #841 was just merged, I think this can be closed.