blob/azureblob: improve error handling#3636
Conversation
vangent
left a comment
vangent
left a comment
There was a problem hiding this comment.
I can't tell if this is an improvement or not, and I no longer have the ability to run tests, so I'd rather not accept it.
If you want to add a specific check to map an error better, I'm likely to be OK with that.
SoMuchForSubtlety
force-pushed
the
azure-blob-more-correct
branch
2 times, most recently
from
dc184e3 to
0ef815c
Compare
- remove duplicated checks - add additional handling for precondition errors
SoMuchForSubtlety
force-pushed
the
azure-blob-more-correct
branch
from
0ef815c to
f005f3e
Compare
vangent
left a comment
vangent
left a comment
There was a problem hiding this comment.
I'm not sure your latest changes addressed my comment.
For example, why are you removing the "bloberror.HasCode" calls? It's not obvious to me whether those are exactly the same as the lower checks (otherwise why would they have been made separate to begin with?).
I think you are just adding mappings for TargetConditionNotMet and SourceConditionNotMet, can you restrict your PR to adding those?
If you want to do a separate refactor that doesn't change any behavior, do that in a separate PR and explain in detail why the behavior doesn't change. Again, I no longer have the ability to run tests against Azure, so my bar for ensuring we're not breaking it is super high.
The duplications were introduced during a migration to a newer client version1. They seem to be accidental. If you look at the implementation of
Sure, I'm fine with creating a separate PR. I'll also run the conformance tests against azure to make sure there is no breakage. Footnotes
|
|
I've removed the handling for additional errors codes and will create a separate PR for it. |
vangent
left a comment
vangent
left a comment
There was a problem hiding this comment.
Can you revert the addition of TestIfNotExist.replay? I don't see any code change in the PR that adds a new test.
The test is defined here, but never ran because running against real azure infra in tests is currently disabled. I re-enabled this locally to get this recording. From now on the test in question can run via this recording, even if no azure credentials are present. I can also split this into a separate PR if you would prefer that. |
|
Ah yes, got it! Thanks. |
2 participants
Follow-up for #3635
This cleans up the error handling and removes code duplication.