Skip to content

refactor: fixes install.sh lint errors and remove blobules publishing code#554

Closed
basvandijk wants to merge 1 commit intomasterfrom
basvandijk/remove-the-deprecated-blobules-publishing
Closed

refactor: fixes install.sh lint errors and remove blobules publishing code#554
basvandijk wants to merge 1 commit intomasterfrom
basvandijk/remove-the-deprecated-blobules-publishing

Conversation

@basvandijk
Copy link
Contributor

@basvandijk basvandijk commented Apr 9, 2020 •
edited by hansl
Loading

This commit contains 3 tasks:

  • refactors the install sh nix rules to be more consistent, and easier to follow. Remove the hack for shellcheck local lint rules and makes sure shellcheck and shfmt are correctly applied to the script.
  • Fixes linting errors that arose during the above step.
  • Remove blobules publishing code. Now that we publish directly to the https://download.dfinity.systems CDN we don't need to publish to blobules anymore. So this commit removes and cleans up any code related to that.

@basvandijk basvandijk force-pushed the basvandijk/remove-the-deprecated-blobules-publishing branch 4 times, most recently from 07df6fe to c797b98 Compare April 9, 2020 20:47
@basvandijk basvandijk marked this pull request as ready for review April 9, 2020 20:54
@basvandijk basvandijk requested a review from a team April 9, 2020 20:54
@basvandijk basvandijk requested a review from a team as a code owner April 9, 2020 20:54
basvandijk
basvandijk commented Apr 9, 2020
View reviewed changes
public/install/020_flags.sh
read_flags() {
# Set values from command line.
# shellcheck disable=SC2199
# https://github.com/koalaman/shellcheck/wiki/SC2199
Copy link
Contributor Author

@basvandijk basvandijk Apr 9, 2020

Choose a reason for hiding this comment

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

@knl do I understand correctly and is the above URL saying I should rewrite the below while loop into a for loop like:

for ARG in "$@"; do

?

Copy link
Contributor

@knl knl Apr 10, 2020

Choose a reason for hiding this comment

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

Yes. Written this way implies there will be only a single iteration of while.

@basvandijk
refactor: remove blobules publishing code
cdd3fc7 
Now that we publish directly to the https://download.dfinity.systems
CDN we don't need to publish to blobules anymore. So this commit
removes and cleans up any code related to that.

This commit also refactors the `install.sh` script generation and
makes sure `shellcheck` and `shfmt` are correctly applied to the
script.
@basvandijk basvandijk force-pushed the basvandijk/remove-the-deprecated-blobules-publishing branch from c797b98 to cdd3fc7 Compare April 9, 2020 21:11
@hansl
Copy link
Contributor

hansl commented Apr 14, 2020

This should be 3 PRs from what I'm reading right now... I really don't like 5 lines of change description ("remove blobules publishing code") that hides ~300 lines. I'll rework the title.

Also if this PR is ready to review please stop force pushing. It makes it very hard to follow what happened since the last comments.

@hansl hansl changed the title refactor: remove blobules publishing code refactor: fixes install.sh lint errors and remove blobules publishing code Apr 14, 2020
nmattia
nmattia reviewed Apr 14, 2020
View reviewed changes
Copy link
Contributor

@nmattia nmattia left a comment

Choose a reason for hiding this comment

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

I agree with @hansl , this should be more than one PR. The refactoring is confusing, I'm not sure what's gone, what's changed and what's new.

ci/ci.nix
, releaseVersion ? "latest"

# TODO: Remove isMaster once switched to new CD system (https://dfinity.atlassian.net/browse/INF-1149)
, isMaster ? true
Copy link
Contributor

@nmattia nmattia Apr 14, 2020

Choose a reason for hiding this comment

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

note: we may re-introduce isMaster in the future for longer-running tests

Copy link
Contributor Author

@basvandijk basvandijk Apr 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I know, but let's add it back when we need it again. That way we'll only add it where we need it. Now it's passed all over the place.

ci/release.nix
@@ -11,10 +11,5 @@ let
ci = import ./ci.nix { inherit src releaseVersion; };
in
if !doRelease then {} else {
Copy link
Contributor

@nmattia nmattia Apr 14, 2020

Choose a reason for hiding this comment

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

are theses release.nix and doRelease things still used?

Copy link
Contributor Author

@basvandijk basvandijk Apr 14, 2020

Choose a reason for hiding this comment

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

Yes they're used to release the SDK.

@basvandijk
Copy link
Contributor Author

basvandijk commented Apr 14, 2020

Good points about the PR size. I'll split it up.

@basvandijk
Copy link
Contributor Author

basvandijk commented Apr 14, 2020

OK I've split this up into the following stacked PRs:

@basvandijk basvandijk closed this Apr 14, 2020
@nmattia
Copy link
Contributor

nmattia commented Apr 14, 2020

Thanks!

dfinity-bot added a commit that referenced this pull request Jan 13, 2021
@dfinity-bot
build: niv advisory-db: update fa47ec0c -> 8e1ad08e
5404bcf 
## Changelog for advisory-db:
Branch: master
Commits: [rustsec/advisory-db@fa47ec0c...8e1ad08e](rustsec/advisory-db@fa47ec0...8e1ad08)

* [`7feb037b`](rustsec/advisory-db@7feb037) RUSTSEC-2020-0017.md (use-after-free in internment) is fixed ([RustSec/advisory-db⁠#554](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/554))
* [`a3efac59`](rustsec/advisory-db@a3efac5) Add "Publish Web" GitHub Action ([RustSec/advisory-db⁠#557](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/557))
* [`50451dd5`](rustsec/advisory-db@50451dd) Publish Web: fix rustsec-admin install ([RustSec/advisory-db⁠#558](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/558))
* [`8e1ad08e`](rustsec/advisory-db@8e1ad08) Publish Web: fix YAML indenting ([RustSec/advisory-db⁠#559](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/559))
mergify bot pushed a commit that referenced this pull request Jan 13, 2021
@dfinity-bot
build: niv advisory-db: update fa47ec0c -> 8e1ad08e (#1290)
ab31923 
## Changelog for advisory-db:
Branch: master
Commits: [rustsec/advisory-db@fa47ec0c...8e1ad08e](rustsec/advisory-db@fa47ec0...8e1ad08)

* [`7feb037b`](rustsec/advisory-db@7feb037) RUSTSEC-2020-0017.md (use-after-free in internment) is fixed ([RustSec/advisory-db⁠#554](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/554))
* [`a3efac59`](rustsec/advisory-db@a3efac5) Add "Publish Web" GitHub Action ([RustSec/advisory-db⁠#557](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/557))
* [`50451dd5`](rustsec/advisory-db@50451dd) Publish Web: fix rustsec-admin install ([RustSec/advisory-db⁠#558](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/558))
* [`8e1ad08e`](rustsec/advisory-db@8e1ad08) Publish Web: fix YAML indenting ([RustSec/advisory-db⁠#559](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/559))
@lwshang lwshang deleted the basvandijk/remove-the-deprecated-blobules-publishing branch July 29, 2022 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nmattia nmattia nmattia left review comments

+1 more reviewer

@knl knl knl left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@basvandijk @hansl @nmattia @knl