Page MenuHomePhabricator

[keyserver] Revert D4422 and D4429
ClosedPublic

Authored by abosh on Jul 13 2022, 12:55 PM.
Tags
None
Referenced Files
F3380227: D4526.diff
Wed, Nov 27, 10:09 PM
Unknown Object (File)
Fri, Nov 22, 11:21 AM
Unknown Object (File)
Sat, Nov 9, 1:21 PM
Unknown Object (File)
Tue, Nov 5, 8:10 PM
Unknown Object (File)
Tue, Nov 5, 8:10 PM
Unknown Object (File)
Tue, Nov 5, 8:10 PM
Unknown Object (File)
Tue, Nov 5, 8:10 PM
Unknown Object (File)
Tue, Nov 5, 8:07 PM
Subscribers

Details

Summary

Relevant Linear issue here. This diff reverts D4422 and D4429 after issues arose with source-nvm.sh resulting in the wrong Node version being used.

Test Plan

Ran the Docker keyserver build and since this is a reverting change, things should work as they did before.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ashoat requested changes to this revision.Jul 13 2022, 2:07 PM
ashoat added inline comments.
keyserver/bash/source-nvm.sh
1 ↗(On Diff #14445)

Let's get rid of this, and remove the executable bit for source-nvm.sh

keyserver/package.json
16 ↗(On Diff #14445)

I don't think . /bash/source-nvm.sh will work

This revision now requires changes to proceed.Jul 13 2022, 2:07 PM
abosh added inline comments.
keyserver/bash/source-nvm.sh
1 ↗(On Diff #14445)

remove the executable bit for source-nvm.sh

Done!

image.png (966×1 px, 260 KB)

Let's get rid of this

ShellCheck complains when you remove the shebang:

image.png (934×1 px, 209 KB)

Tips depend on target shell and yours is unknown. Add a shebang.

Instead, I can change it to sh? But I don't think that change is necessary since this code will run in either bash or sh.

keyserver/package.json
16 ↗(On Diff #14445)

I think you're right, this change just reverted it to what it was before but as you can see that command fails in the keyserver directory:

image.png (966×1 px, 230 KB)

abosh marked an inline comment as done.

Removed executable bit from source-nvm.sh. Addressed rest of inline feedback in comments below.

ashoat requested changes to this revision.Jul 14 2022, 6:36 AM
ashoat added inline comments.
keyserver/package.json
16 ↗(On Diff #14452)

It still is . /bash/source-nvm.sh

This revision now requires changes to proceed.Jul 14 2022, 6:36 AM
keyserver/bash/source-nvm.sh
1 ↗(On Diff #14445)

Okay fair. I still think it would be better to replace with sh then... if it's written for sh, it's more likely to work for bash than the reverse (being written for bash and working for sh)

keyserver/package.json
16 ↗(On Diff #14445)

this change just reverted it to what it was before

Not sure what you mean, the left side of D4422 clearly shows a working command

Address inline feedback:

  • Replaced bash with sh
  • Replaced all usages of . /bash/source-nvm.sh with . bash/source-nvm.sh. I'm not sure why this diff didn't revert the first time, I was wrong about that.

Test Plan:

  • Searched for all usages of source-nvm.sh and made sure they didn't have a forward slash at the beginning/were executable.
  • Ran the Docker keyserver build and it worked with no errors.
  • Ran ShellCheck on source-nvm.sh.

iOS build is failing, but that should be unrelated...planning changes to investigate

Awesome, looks great!!

This revision is now accepted and ready to land.Jul 14 2022, 8:07 AM
This revision was automatically updated to reflect the committed changes.