Skip to content
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

chore: use ooniprobe-cli 3.11.0-beta.2 #260

Merged
merged 2 commits into from
Dec 6, 2021
Merged

chore: use ooniprobe-cli 3.11.0-beta.2 #260

merged 2 commits into from
Dec 6, 2021

Conversation

bassosimone
Copy link
Contributor

This diff changes ooniprobe-desktop to use cli v3.11.0-beta.2.

This cli release includes changes in the last (at this point six!)
months to improve measurements quality.

It is missing some extra fixes required to bless a stable
release. Chiefly among them, the possiblity of running
cleanly the DNSCheck experiment.

I'm going to work on these issues as soon as possible.

In the meanwhile, this release is a good testing base to check
whether we have additional lingering issues.

A few notes to explain this diff follows.

First, I took the liberty of reducing as much as possible
the amount of markdown linting warning I did see.

Second, 3.11.0-beta.2 is the first release that is fully built
using the cloud. I have not added any PGP key to the cloud
as this seems a bit futile. Maybe it is not and maybe I'm not
seeing the full picture, so we should discuss this topic. (I
would not put my PGP key in there, and instead I'd put another
key that has no password to sign releases; is this better
than not using any key at all?)

Third, cloud builds do not use tar.gz anymore. I did this to
work around ooni/probe#1884 (in general,
less pieces we use, less errors could occur.)

As a result, the download script needed a bit of reworking.

Fourth, while I reckon this is unlikely, I couldn't help noticing
that the previous script did not download in case files were
already on disk. To add a bit of robustness to this process, I
am now prefixing the downloaded file with its version. This way,
we'll re-download if there is a cli version change and there
are already files on disk and, by some other mistake, the build
rules are such that we don't clean existing files.

The related issue is ooni/probe#1879

This diff changes ooniprobe-desktop to use cli v3.11.0-beta.2.

This cli release includes changes in the last (at this point six!)
months to improve measurements quality.

It is missing some extra fixes required to bless a stable
release. Chiefly among them, the possiblity of running
cleanly the DNSCheck experiment.

I'm going to work on these issues as soon as possible.

In the meanwhile, this release is a good testing base to check
whether we have additional lingering issues.

A few notes to explain this diff follows.

First, I took the liberty of reducing as much as possible
the amount of markdown linting warning I did see.

Second, 3.11.0-beta.2 is the first release that is fully built
using the cloud. I have not added any PGP key to the cloud
as this seems a bit futile. Maybe it is not and maybe I'm not
seeing the full picture, so we should discuss this topic. (I
would not put my PGP key in there, and instead I'd put another
key that has no password to sign releases; is this better
than not using any key at all?)

Third, cloud builds do not use tar.gz anymore. I did this to
work around ooni/probe#1884 (in general,
less pieces we use, less errors could occur.)

As a result, the download script needed a bit of reworking.

Fourth, while I reckon this is unlikely, I couldn't help noticing
that the previous script did not download in case files were
already on disk. To add a bit of robustness to this process, I
am now prefixing the downloaded file with its version. This way,
we'll re-download if there is a cli version change and there
are already files on disk and, by some other mistake, the build
rules are such that we don't clean existing files.

The related issue is ooni/probe#1879
scripts/download-bin.js Outdated Show resolved Hide resolved
Copy link
Contributor

@sarathms sarathms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bassosimone! The changes look good to me. I added a commit to do some JS stuff you asked for in a comment.

I am yet to test a desktop app build with this version of probe-cli. I will come back here and update the review when I have done that tomorrow.

Copy link
Contributor

@sarathms sarathms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the build with this version of probe-cli and it works as expected 🐳

@sarathms sarathms merged commit cda481a into master Dec 6, 2021
@sarathms sarathms deleted the issue/1879 branch December 6, 2021 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants