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

[Bug?]: Setting --verbose when deploying to baremetal "fails" during cleanup step. SshExecutor.js does not check that "args" is defined. #10654

Closed
1 task
hjalmarhoglund opened this issue May 21, 2024 · 6 comments · Fixed by #10663
Labels
bug/needs-info More information is needed for reproduction

Comments

@hjalmarhoglund
Copy link

hjalmarhoglund commented May 21, 2024

What's not working?

When deploying to baremetal and setting the verbose flag,

yarn rw deploy baremetal production --verbose

all goes well until the last step, the cleanup step. The following output is produced:

[STARTED] myapp.com
[STARTED] Connecting...
...
[COMPLETED] Restarting api process...
[STARTED] Cleaning up old deploys...
[FAILED] Cannot read properties of undefined (reading 'join')
[FAILED] Cannot read properties of undefined (reading 'join')

Notably, the deploy doesn't actually fail. The app is re-deployed and everything works. If you deploy without the verbose flag,

yarn rw deploy baremetal production

no error occurs.

I believe that the error comes from packages/cli/src/commands/deploy/baremetal/SshExecutor.js, where on lines 21 and 31 no check if args is undefined is done. I believe this is the case since the error only occurs when using the verbose flag.

How do we reproduce the bug?

Run yarn rw deploy baremetal production --verbose towards your baremetal server.

What's your environment? (If it applies)

On the system I am deploying from:

  System:
    OS: macOS 14.4.1
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.11.1 - /private/var/folders/_0/_dkb2pjx2910hmkb_3822c5w0000gn/T/xfs-72799c62/node
    Yarn: 3.7.0 - /private/var/folders/_0/_dkb2pjx2910hmkb_3822c5w0000gn/T/xfs-72799c62/yarn
  Databases:
    SQLite: 3.43.2 - /usr/bin/sqlite3
  Browsers:
    Safari: 17.4.1
  npmPackages:
    @redwoodjs/core: 7.5.1 => 7.5.1 
  redwood.toml:
    [web]
      title = "MyApp"
      port = 8910
      apiUrl = "/.redwood/functions"
      includeEnvironmentVariables = []
    [api]
      port = 8915
    [browser]
      open = true
    [notifications]
      versionUpdates = ["latest"]


On the system I am deploying to:

  System:
    OS: Linux 5.15 Ubuntu 22.04.4 LTS 22.04.4 LTS (Jammy Jellyfish)
    Shell: 5.1.16 - /bin/bash
  Binaries:
    Node: 20.13.1 - /tmp/xfs-6d0da2fd/node
    Yarn: 3.7.0 - /tmp/xfs-6d0da2fd/yarn
  npmPackages:
    @redwoodjs/cli-data-migrate: 7.5.1 => 7.5.1 
    @redwoodjs/core: 7.5.1 => 7.5.1 
  redwood.toml:
    [web]
      title = "MyApp"
      port = 8910
      apiUrl = "/.redwood/functions"
      includeEnvironmentVariables = []
    [api]
      port = 8915
    [browser]
      open = true
    [notifications]
      versionUpdates = ["latest"]

Are you interested in working on this?

  • I'm interested in working on this
@hjalmarhoglund hjalmarhoglund added the bug/needs-info More information is needed for reproduction label May 21, 2024
@ahaywood
Copy link
Contributor

@hjalmarhoglund Thanks for reporting this. I'm going to tag in another team member who specializes in deployment and will get back o you!

Josh-Walker-GM added a commit that referenced this issue May 21, 2024
…10663)

**Problem**
We recently improved the verbose output of the baremetal deployment
process. It looks like this improvement has introduced a small bug. See
#10654 for more details.

**Changes**
1. Guard against `args` being undefined.

---------

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Josh-Walker-GM added a commit that referenced this issue May 22, 2024
…10663)

**Problem**
We recently improved the verbose output of the baremetal deployment
process. It looks like this improvement has introduced a small bug. See
#10654 for more details.

**Changes**
1. Guard against `args` being undefined.

---------

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Josh-Walker-GM added a commit that referenced this issue May 22, 2024
…10663)

**Problem**
We recently improved the verbose output of the baremetal deployment
process. It looks like this improvement has introduced a small bug. See
#10654 for more details.

**Changes**
1. Guard against `args` being undefined.

---------

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
@Josh-Walker-GM
Copy link
Collaborator

Hey @hjalmarhoglund, would you mind trying out v7.6.1 and confirm that the fix works?

@hjalmarhoglund
Copy link
Author

Hey @Josh-Walker-GM! Sure, I've upgraded and tested deploying. I believe another error has been introduced. This time, the error affects both the verbose and the none-verbose baremetal deploys.

Running,

yarn rw deploy baremetal production --verbose

gives the output

...
[STARTED] Cloning `main` branch...
SshExecutor::exec running command `gitclone --branch=main --depth=1 git@github.com:hjalmarhoglund/myapp.git 20240522150326` in /var/www/myapp
[FAILED] Error while running command `gitclone --branch=main --depth=1 git@github.com:hjalmarhoglund/myapp.git 20240522150326` in /var/www/myapp
[FAILED] bash: line 1: gitclone: command not found

Running,

yarn rw deploy baremetal production

gives the output

Error while running command `gitclone --branch=main --depth=1 git@github.com:hjalmarhoglund/myapp.git 20240522150713` in /var/www/myapp
bash: line 1: gitclone: command not found
...

The problem

The problem is that git clone [...] becomes gitclone [...].

Looking at the new code for SshExecutor.js we find on lines 13 and 14,

const argsString = args?.join(' ') || ''
const sshCommand = command + argsString

which does not correctly set a space between the command and the first arg.

Proposed fix

I think this could be solved by changing line 13 to:

const argsString = args ? ' ' + args.join(' ') : ''

Testing this in a local Node.js v20.11.1 session:

function testFix(command, args) {
    const argsString = args ? ' ' + args.join(' ') : ''
    const sshCommand = command + argsString
    // See what we end up with
    console.log(sshCommand)
}
const command = 'git'
const args1 = ['clone', '--branch=main']
const args2 = undefined
// Now running the proposed expression of argsString towards both args1 and args2
testFix(command, args1)
testFix(command, args2)

Gives the output

git clone --branch=main
git

Testing if this solves deploying

I try implementing my fix into node_modules/@redwoodjs/cli/dist/commands/deploy/baremetal/SshExecutor.js and run the deploy scripts, both verbose and non-verbose. For me, everything works.

...
async exec(path, command, args) {
    //const argsString = args?.join(" ") || "";
    const argsString = args ? ' ' + args.join(' ') : ''
    const sshCommand = command + argsString;
...

Hope this is of help, and thank you for getting to this as quickly as you did! Let me know if you want me to test anything again.

@Josh-Walker-GM
Copy link
Collaborator

Thanks for testing this so quickly! I agree we've messed up and introduced an even worse error! I'll get a fix and patch out again for this today

@Josh-Walker-GM
Copy link
Collaborator

Okay v7.6.2 is out with the further fix. Hopefully we didn't break it even more haha!

@hjalmarhoglund
Copy link
Author

Fantastic! I've tested deploying on v7.6.2. Both verbose and non-verbose works like a charm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/needs-info More information is needed for reproduction
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants