Add flag to upload coverage insecurely

[toddmazierski]

Add an --insecure flag to both the after-build and upload-coverage sub-commands to upload test coverage insecurely (without HTTPS).

This is not recommended for general use. This is intended for use in private environments where the benefits of secure transfer outweigh the operational costs.

[CLAassistant]

All committers have signed the CLA.

[ale7714]

This looks good! But I also pair on it so I think it's worth having some different 👀 .

[dblandin]

Should we always set the scheme on the parsed url? https normally but if the insecure flag is set, http?

[toddmazierski]

Ah, interesting idea. That would make the code simpler (we could remove the guard clause).

That change might give the impression to readers, though, that the scheme we're receiving from the API is invalid or missing. It's not, really—we're just overriding it in this very specific mode of operation. But, of course, realistically there's only two possibilities: https or http. 😛

I could go either way. 😄 Do you favor one way or the other?

[dblandin]

I kind of like that regardless of the URL scheme returned via the API, that we would be enforcing secure transport by default and only disabling it if the insecure flag is used.

Up to you though. Fine with whichever you prefer!

[toddmazierski]

I kind of like that regardless of the URL scheme returned via the API, that we would be enforcing secure transport by default and only disabling it if the insecure flag is used.

I dig it! I'll make that change. 👍

[ale7714]

After some conversation, I feel pretty good with the PR and how the change is very encapsulated.

[dblandin]

If this is an unusable value, I guess the reporter will fail further downstream at the http post request?

[toddmazierski]

In case it wasn't obvious, I'm new to Go. So this might not be desirable. 😄 Would you recommend a panic instead?

[dblandin]

I'm not familar with all of the error handling in place but it looks like you could do something like this if you wanted to handle the url parsing error directly:

func (u Uploader) TransformPostBatchURL(rawUrl string) (string, errror) {
  parsed, err := url.Parse(rawUrl)

  if err != nil {
    return nil, err
  }
  // snip
}

and change the calling code to:

postBatchURL, err := u.TransformPostBatchURL(batchLinks.Links.PostBatch)

if err != nil {
  return errors.WithStack(err)
}

return u.SendBatches(testReport, postBatchURL)

[ale7714]

FWIW i thought about this but having an unusable URL it's extremely unlikely and I think this a nice to have but not super necessary. Personally I find super annoying those if 🙈

[toddmazierski]

Ah, OK!

It's unlikely, @ale7714, but @dblandin's suggestion means it'll be much easier to discover and debug.

[dblandin]

One quick comment/question. LGTM!