Feature: allow to pass the image URL directly to the devices

[b4u-mw]

Fix for #399

[b-rowan]

Seems fine to me, simple enough fix.

[b-rowan]

Can you squash those commits into one, then I will merge.

[easybe]

I don‘t quite understand why this is needed. We already support hosting files on an external server without having to go through gooseBit to download them.
If that is no longer the case, the CDN use case was broken, which is bad. We should not need to introduce a new configuration option.

[easybe]

I see, this was broken when S3 support was introduced (b16bb2c). If S3 needs special behaviour, then it should be activated when S3 is chosen as a backend. Maybe @jkuri can help us here.

[b-rowan]

I see, this was broken when S3 support was introduced (b16bb2c). If S3 needs special behaviour, then it should be activated when S3 is chosen as a backend. Maybe @jkuri can help us here.

I would have to look, but any backend shouldn't change the behavior of the remote server, so I would assume this is a regression.

[easybe]

I commented on the issue: #399 (comment). @jkuri are you around to help us out here?

[jkuri]

I commented on the issue: #399 (comment). @jkuri are you around to help us out here?

I'll check later today, but as far as I recall I did fix the CDN issue later after this was introduced.

[easybe]

@jkuri I answered here. Let's move the conversation to the issue.

[jkuri]

I am not sure what is wrong with this fix @easybe? I would rather just simplify it without additional props in configuration;

parsed_uri = urlparse(software.uri)
if parsed_uri.scheme in ("http", "https"):
    href = software.uri
else:
    # this still handles files prefixed with s3:// or file:// like before
    href = str(request.url_for("download_artifact", dev_id=device.id))

[easybe]

I am not sure what is wrong with this fix @easybe? I would rather just simplify it without additional props in configuration;

parsed_uri = urlparse(software.uri)
if parsed_uri.scheme in ("http", "https"):
    href = software.uri
else:
    # this still handles files prefixed with s3:// or file:// like before
    href = str(request.url_for("download_artifact", dev_id=device.id))

Yes, that looks fine to me. Definitely no additional setting for this.

[b4u-mw]

I have switched to the more simple approach you suggested.

[jkuri]

LGTM 👍

[b4u-mw]

I added a few tests now

[b4u-mw]

I added a few tests now

[easybe]

LGTM, thanks!

[easybe]

But please fix the CI issues and squash into 1 commit.

[b-rowan]

CI issues are all mypy in the test files, should be pretty easy to fix. Make sure you have the pre-commit hooks installed (pre-commit install), and then you can check them with pre-commit run --all-files.

[easybe]

Don’t merge master into the branch! Please, rebase instead.

[jkuri]

@b4u-mw if you need help with rebasing, this should do the trick;

git reset --hard 516cbff380c71f5184621e38dc1a5d3e5bbd4c36
git remote add upstream https://github.com/UpstreamDataInc/goosebit
git fetch upstream
git rebase upstream/master
git push origin feature/deliver-remote-url -f

[b4u-mw]

Don’t merge master into the branch! Please, rebase instead.

Fixed. This was not intentionally, I just pressed the "Sync with master" button and expected a rebase.

[easybe]

Don’t merge master into the branch! Please, rebase instead.

Fixed. This was not intentionally, I just pressed the "Sync with master" button and expected a rebase.

Yeah, I hate that it does that by default. One has to explicitly select “with rebase”.