Use declarative port mappings and copy files values when CLI flags are empty

[onatm]

Fixes #795

[onatm]

I've just found the following assertions are completely wrong:

ignite/cmd/ignite/run/create_test.go

Lines 285 to 293 in 3120baf

	// Check if port mappings are set as expected.
	if reflect.DeepEqual(rt.createFlag.VM.Spec.Network.Ports, rt.wantPortMapping) {
	t.Errorf("expected VM.Spec.Network.Ports to be %v, actual: %v", rt.wantPortMapping, rt.createFlag.VM.Spec.Network.Ports)
	}
	// Check if the SSH values are set as expected.
	if reflect.DeepEqual(rt.createFlag.VM.Spec.SSH, rt.wantSSH) {
	t.Errorf("expected VM.Spec.SSH to be %v, actual: %v", rt.wantSSH, rt.createFlag.VM.Spec.SSH)
	}

[onatm]

TestApplyVMFlagOverrides is written to validate the wrong implementation and I strongly recommend to rewrite it.

TBH, I cannot see any value in TestApplyVMFlagOverrides because it is not actually testing the override logic as it happens in the CLI.

[darkowlzz]

Thanks for reporting and fixing this bug.

TBH, I cannot see any value in TestApplyVMFlagOverrides because it is not actually testing the override logic as it happens in the CLI.

Some background for applyVMFlagOverrides() would help understand it better 🙂
Before this, we used to have TestConstructVMFromCLI(), that did exactly what you're looking for.
applyVMFlagOverrides() was introduced to support multiple layers of configurations - component config (global ignite configuration), VM config (set via --config run command flag) and flag configuration (like --copy-flags and --ports). The base VM that this function receives is after combing the component config and VM config. This function tries to capture the user's intent expressed via flags to set the properties of the VM. The flags in ignite run command is a combination of simple types and VM spec type. The types that are complex to be in flag have a simpler form that is overwritten in the VM spec. CreateFlag contains api.VM for VM and separate fields for SSH, PortMappings and CopyFiles. api.VM contains ssh, portmappings and copyfiles, but aren't flag friendly so they have separate types to simplify that.
Because of this combination of api.VM, simple types for some properties, and merging of base configurations, it's difficult to find out the user's intent in the baseVM and the CreateFlags. One way to know if the user specified a flag is to do fs.Changed() on the fields that the users can set and are also part of api.VM. When a change is found, the value in baseVM is overwritten by the flag value.
In case of CopyFiles and PortMappings, since they have separate variables for their flags, we don't need to do fs.Changed(). If there's any value, we should just use it. So just checking their length before parsing them should fix that issue.
Hope this helps understand how we have to apply the baseVM VM configurations from two different sources with the VM config expressed via the CLI flags.

Regarding the tests, thanks for noticing the incorrect assertions and fixing them and improvements in the comparison.
I would recommend adding a few cases as follows to validate that the CopyFiles and PortMappings set in the baseVM from a config file don't get overwritten by empty flags for them:

                {
                        name: "copy files in baseVM",
                        createFlag: &CreateFlags{
                                VM: &api.VM{
                                        Spec: api.VMSpec{
                                                CopyFiles: []api.FileMapping{
                                                        {
                                                                HostPath: "/tmp/foo",
                                                                VMPath:   "/tmp/bar",
                                                        },
                                                },
                                        },
                                },
                        },
                        wantCopyFiles: []api.FileMapping{
                                {
                                        HostPath: "/tmp/foo",
                                        VMPath:   "/tmp/bar",
                                },
                        },
                },
...
                {
                        name: "port mapping in base VM",
                        createFlag: &CreateFlags{
                                VM: &api.VM{
                                        Spec: api.VMSpec{
                                                Network: api.VMNetworkSpec{
                                                        Ports: meta.PortMappings{
                                                                {
                                                                        BindAddress: net.IPv4(0, 0, 0, 0),
                                                                        HostPort:    uint64(80),
                                                                        VMPort:      uint64(80),
                                                                        Protocol:    meta.ProtocolTCP,
                                                                },
                                                        },
                                                },
                                        },
                                },
                        },
                        wantPortMapping: meta.PortMappings{
                                meta.PortMapping{
                                        BindAddress: net.IPv4(0, 0, 0, 0),
                                        HostPort:    uint64(80),
                                        VMPort:      uint64(80),
                                        Protocol:    meta.ProtocolTCP,
                                },
                        },
                },

With these and your improvements in the test assertions, we should have the right tests that covers this bug.
Maybe we should also support appending the port mappings and copy files from the config file and CLI, but we can work on that separately.

Hope that helps understand it better.

[darkowlzz]

Checking the length for both CopyFiles and PortMappings should be enough to get this right.
"copy-files" and "ports" are not directly taken from the VM spec. They have separate variables for simpler flag friendly types, refer:

ignite/cmd/ignite/cmd/vmcmd/create.go

Lines 71 to 72 in 3120baf

	fs.StringSliceVarP(&cf.PortMappings, "ports", "p", cf.PortMappings, "Map host ports to VM ports")
	fs.StringSliceVarP(&cf.CopyFiles, "copy-files", "f", cf.CopyFiles, "Copy files/directories from the host to the created VM")

.

[onatm]

I am going to drop the commit c5d81b9

[onatm]

@darkowlzz for your detailed explanation about how ignite handles different sets of configurations. Now I have a better understanding of how the config values flow through the application 👍 I'll make the changes you recommended and finish working on this PR.

[onatm]

I've just pushed the changes you recommended @darkowlzz Thank you for the review!

[darkowlzz]

Thanks a lot 🙂
LGTM!