Fixes a crash that happens when there is no account in the config

[JanLukasGithub]

This made it impossible to run without the config file

[JanLukasGithub]

I will add some tests...

[nvthongswansea]

@JanLukasGithub I think you could do this instead: Put this

if accountIndex == -1 { 
   return fmt.Errorf("account not found")
}

above the line ac = LoadEnvVariables(ac). Then in the test, you could do assert.NotNil(t, err), which makes more sense of "if you choose wrong account, there will be an error".

[JanLukasGithub]

There are commands you don't need an account for, like make-config and version, where you don't need/can't have an account

[nvthongswansea]

@JanLukasGithub btw, instead of separating the unit tests, combine these Test_SelectAccount, Test_SelectAccountSettings, and Test_EmptyConfig into one called Test_NewRuntime; and implement test cases. Using Test_NewRuntime tells you what function/method you are testing.

[nvthongswansea]

There are commands you don't need an account for, like make-config and version, where you don't need/can't have an account

In line 231, we do have

if len(conf.Accounts) > 0 && accountIndex == -1 {
        if !CommandWithoutConfig(os.Args) {
            return nil, fmt.Errorf("account '%s' does not exist", accountName)
        }
    }

just remove len(conf.Accounts) > 0, and do accountIndex == -1 && !CommandWithoutConfig(os.Args).

[nvthongswansea]

Ah, you want to to get rid of the case accountIndex == -1 and the command doesn't need the config. Sorry, misunderstanding.

[JanLukasGithub]

I think this is a bit better

[JanLukasGithub]

@JanLukasGithub btw, instead of separating the unit tests, combine these Test_SelectAccount, Test_SelectAccountSettings, and Test_EmptyConfig into one called Test_NewRuntime; and implement test cases. Using Test_NewRuntime tells you what function/method you are testing.

What I would now think of is doing a method Test_NewRuntime which calls those 4 other functions, or else you get 1 function that's far to big and not readable, but even one big function would be better than my approach I guess

[JanLukasGithub]

I think the problem is that the environment variables are loaded inside this function which makes it multi-purpose and hard to test properly as there would be a lot of really different test cases, like one test case has to test environment variables, while the others shouldn't have them set at all

[nvthongswansea]

@JanLukasGithub btw, instead of separating the unit tests, combine these Test_SelectAccount, Test_SelectAccountSettings, and Test_EmptyConfig into one called Test_NewRuntime; and implement test cases. Using Test_NewRuntime tells you what function/method you are testing.

What I would now think of is doing a method Test_NewRuntime which calls those 4 other functions, or else you get 1 function that's far to big and not readable, but even one big function would be better than my approach I guess

The truth is using test cases in a sinhle unit test function is more compact (less code) and easier to manage. You don't have to call those 4 functions either.

[nvthongswansea]

Other than the test, lgtm.

[nvthongswansea]

@JanLukasGithub Example for multiple test cases testing: https://github.com/gridscale/packer-plugin-gridscale/blob/main/builder/gridscale/builder_test.go

[bkircher]

Or maybe this helps: https://dave.cheney.net/2013/06/09/writing-table-driven-tests-in-go

[nvthongswansea]

LGTM

[bkircher]

Hey @nvthongswansea if it's a LGTM, just merge it. No reason to keep these open 😄