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

Fix null reference when no env vars defined in yml #25

Merged
merged 6 commits into from
Aug 19, 2020
Merged

Conversation

davidsiaw
Copy link
Contributor

@davidsiaw davidsiaw commented Apr 2, 2020

This PR fixes a bug introduced in #23 where if you do not define a run_env section in the barcelona.yml it would throw a null reference error.

How to test

  1. Create a barcelona.yml containing this:
environments:
  hello:
    name: hello
    image_name: quay.io/dsiawdegica/hello
    scheduled_tasks: []
    services:
      - name: web
        service_type: web
        cpu: null
        memory: 256
        command: sh /start.sh
        listeners:
          - endpoint: hello
            health_check_path: /
        port_mappings:
          - lb_port: 80
            container_port: 8080
            protocol: http

2.Run ./bcn -d run -e hello -E FOO=bar ew
3. Observe that there is no segfault error, and it outputs

{"command":"ew","env_vars":{"FOO":"bar"},"interactive":true}

@degicat
Copy link

degicat commented Apr 3, 2020

@Resonious please review this

@degicat degicat requested a review from Resonious April 3, 2020 01:23
Copy link
Member

@Resonious Resonious left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I have a quick code question!

cmd/run.go Outdated Show resolved Hide resolved
@Resonious
Copy link
Member

@davidsiaw I downloaded the binary from the CI build job https://gitlab.com/degica/barcelona-cli/-/jobs/504621079 and it still seems to segfault after performing the test instructions.

[nigel@resonious testsz]$ cat barcelona.yml 
environments:
  hello:
    name: hello
    image_name: quay.io/dsiawdegica/hello
    scheduled_tasks: []
    services:
      - name: web
        service_type: web
        cpu: null
        memory: 256
        command: sh /start.sh
        listeners:
          - endpoint: hello
            health_check_path: /
        port_mappings:
          - lb_port: 80
            container_port: 8080
            protocol: http

[nigel@resonious testsz]$ ./bcn -d run -e hello -E FOO=bar ew
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x776392]

goroutine 1 [running]:
github.com/degica/barcelona-cli/cmd.loadEnvVars(0x7ffc2466a771, 0x5, 0x0, 0x0, 0xc000115b90, 0x68, 0x1)
        /go/src/github.com/degica/barcelona-cli/cmd/run.go:163 +0xa2
github.com/degica/barcelona-cli/cmd.glob..func38(0xc000136b40, 0x100, 0xc000136b40)
        /go/src/github.com/degica/barcelona-cli/cmd/run.go:52 +0x1ba
github.com/degica/barcelona-cli/vendor/github.com/urfave/cli.HandleAction(0x7c6d20, 0x85f708, 0xc000136b40, 0xc000026600, 0x0)
        /go/src/github.com/degica/barcelona-cli/vendor/github.com/urfave/cli/app.go:485 +0xc8
github.com/degica/barcelona-cli/vendor/github.com/urfave/cli.Command.Run(0x842001, 0x3, 0x0, 0x0, 0x0, 0x0, 0x0, 0x851c3b, 0x28, 0x0, ...)
        /go/src/github.com/degica/barcelona-cli/vendor/github.com/urfave/cli/command.go:193 +0x97c
github.com/degica/barcelona-cli/vendor/github.com/urfave/cli.(*App).Run(0xc00010b6c0, 0xc000012080, 0x8, 0x8, 0x0, 0x0)
        /go/src/github.com/degica/barcelona-cli/vendor/github.com/urfave/cli/app.go:250 +0x82a
main.main()
        /go/src/github.com/degica/barcelona-cli/main.go:41 +0x445
[nigel@resonious testsz]$

Let me know if there's something I'm missing.

@davidsiaw
Copy link
Contributor Author

@Resonious That is embarassing. I must have messed up. I made a fix.

Copy link
Member

@Resonious Resonious left a comment

Choose a reason for hiding this comment

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

@davidsiaw All good - it no longer segfaults, but I have one more code question.

if err != nil {
return cli.NewExitError(err.Error(), 1)
}
heritageName = env.Name
Copy link
Member

Choose a reason for hiding this comment

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

Looks to me like we might still need this heritageName = env.Name part.

Later in the file, it does this

api.DefaultClient.Post("/heritages/"+heritageName+"/oneoffs", bytes.NewBuffer(j))

which might not won't work right if len(envName) > 0 && len(heritageName) == 0.

Does that sound right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm that code looks awfully suspicious. You are right. I'll try and fix it in a sane way

@davidsiaw
Copy link
Contributor Author

@Resonious sorry it took me forever to get back to this. I legitimately thought that this was solved but it wasn't. I made a change to fix that issue. I will make an issue to refactor this into a more understandable thing later.

@davidsiaw
Copy link
Contributor Author

@Resonious I have rebased the branch against master and fixed the ci, if you have time could you please have another look? I made an issue to fix this messiness in run.go #29

Copy link
Member

@Resonious Resonious left a comment

Choose a reason for hiding this comment

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

No problem - just one more tiny thing, then it should be good to go

cmd/run.go Outdated Show resolved Hide resolved
@davidsiaw davidsiaw requested a review from Resonious August 19, 2020 03:52
@davidsiaw
Copy link
Contributor Author

@Resonious made some fixes to this long-overdue pr. Could you have one last look? Thanks!

Copy link
Member

@Resonious Resonious left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Resonious Resonious merged commit f184bd1 into master Aug 19, 2020
@davidsiaw davidsiaw deleted the fix-nullref branch August 19, 2020 05:28
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.

3 participants