-
Notifications
You must be signed in to change notification settings - Fork 825
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
blob.go: adapt code to use helpers for paths #19
Conversation
I actually have a better plan. Do you mind creating methods like |
Well now that is better alright! |
@ahmetalpbalkan are you ok with these methods being called, and the results used directly without any intermediate " so eg.
instead of
Also, what would you have these be methods of? StorageClient? BlobStorageClient? or should they be generic methods, and be put in util.go? |
yeah I'm ok (as long as there's nothing complicated on that line except calling pathForBlob), these can go to blob.go as global non-exported functions. there's no need to make it functions of BlobStorageClient. oh also, unit tests for those can go to blob_test.go |
Actually we dont need a |
you actually do. what happens when tomorrow we want to address containers as |
eh... well I guess you're right. I was focussed on the present. :} |
@ahmetalpbalkan , this is what I have so far (minus the tests). Let me know what you think. Note: I've added a (And of course, the current tests pass with this code). |
Nope please don't add that. That duplicates the job of net/url package and might have introduced bugs elsewhere. I wouldn't do that unless it's very necessary. (Did you also run the test suite with this change?) |
Yes I did (added that in my comment after I first posted it). I don't think it really duplicates the work of the code in net/url, though. I see that line (in net/url as I pointed) as just a precaution, or "helper" of sorts ("if the slash isn't prepended, we'll prepend it"). Paths are after all meant to start with a |
Okay I guess it's okay unless it makes requests with double slashes (//) or generates GetBlobURL or SAS URL like that. These are already covered by the tests. I will take a final look and then merge. Thanks @jf. |
Oh and I realized pathForBlob can reuse pathForContainer. 😄 also having simple unit tests would be really great. Thanks. |
ha, I never really considered that! Do you want to keep them separate, though? or are we worrying too much? I'll get some tests in. |
ok I've just added the tests. Had to figure out (took some troubleshooting!) why no matter what I did, the tests I added weren't running. So it turns out I found a bug in blob_test.go, and fixed that too. I'm choosing to keep the fix in a separate commit, because I think it's important enough to point out. Prior to this, we weren't running the test for blobSASStringToSign() (and it was wrong to boot, too), apparently! |
func Test_pathForBlob(t *testing.T) { | ||
out := pathForBlob("foo", "blob") | ||
if expected := "/foo/blob"; out != expected { | ||
t.Errorf("Wrong pathForContainer. Expected: '%s', got: '%s'", expected, out) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pathForBlob
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:}
@jf can you sign a CLA at https://cla.msopentech.com/ ? |
Please rebase on top of master to include the CI commit, otherwise test will fail. |
@jeffmendoza is there a more specific privacy policy for developers/open source contributors? I don't see how getting my address, for example, is necessary |
Looks like there is a link at the end of the page to here: https://msopentech.com/privacy-policy/ |
I'm aware and have already looked through it. That's a generic one for all of "MS Open Tech" - "Msopentech.com and other Microsoft Open Technologies, Inc. (“MS Open Tech”) websites and services of MS Open Tech that collect data and display these terms, as well as its offline product support services.". Perhaps some of these will have a legitimate reason for asking for an address.... but as a "pure github contributor and nothing else", I see no reason for Microsoft to ask for my address. |
Anyway, I've filled in the form. But haven't received any email yet. Is it automated? or manual? (so ie. a delay is normal) |
@hopetobelievein is CLA approved for @jf ? |
@jf Looks like our new system just took over, can you try: https://cla2.msopentech.com/ |
Ok I'm going to give it one more try. But if this doesn't go through, I'm out for submitting PRs. |
@jeffmendoza is CLA approved for @jf? |
I am approved already, @ahmetalpbalkan . But dunno if this is good enough for you, or if you're required to hear from one of the folks you've asked first (in which case, we need to wait on them). |
@jf ah okay, it is just I don't have access to the interface that shows if you're approved or not. I did not realize you got the approval email from the context above. |
blob.go: use helper methods for creating paths
@jf Thanks for bearing with us |
Fix build
No description provided.