added additional rpm handling

[dbw7]

Very open to feedback on this

[atanasdinov]

Looks good to me. There are several suggestions that we'd need to take care of but nothing major in those - good job!

One thing to mention is that there's no documentation around the /rpms directory in the README and you might also want to sync with @ipetrov117 since he's also looking into how RPMs will be handled.

[ipetrov117]

I am not sure what use we get from moving the b.combustionDir property to rpmDestDir and using it in copyRPMs. Can't we just use b.combustionDir directly as a parameter to the copyRPMs function? That way we will save up one more line of code that we have to write without making the existing code too complex.

[dbw7]

I mainly added this so that it's clear that there was a variable that held where the rpms should go in the end, but like you said we can save that line by omitting it, will update 👍

[ipetrov117]

Looks good to me. There are several suggestions that we'd need to take care of but nothing major in those - good job!

One thing to mention is that there's no documentation around the /rpms directory in the README and you might also want to sync with @ipetrov117 since he's also looking into how RPMs will be handled.

The doc around the /rpms directory would be of great help both to me and to anyone who is getting familiar with the EIB project.

Regarding the RPM handling, the suggested changes here offer a good base line from where we could continue with the air-gapped RPM handling. Granted, some files would have to be changed (the rpm_install.sh script for instance), but most of the code here can be reused. IMO this PR is a step in the right direction.

[ipetrov117]

One more thing that got me wondering looking at the script execution comment that @atanasdinov made. Does it make sense for us to start naming our scripts with XX_ prefix as @dbw7 has done here? IMO this would provide additional visibility over the flow of the script execution and will make it easier to track especially when we have more scripts. Furthermore this will decrease any uncertainty of where in the flow to add a new script.

That said, I still do not have the full picture so I might be wrong here. @atanasdinov @jdob @dbw7 wdyt?

[atanasdinov]

You're completely correct.

My comment was indeed to point out that the introduced XX_ scheme in this PR will conflict with the initial alphabetical sorting (which was put there just for determinism purposes). I don't see any issues with starting here and adjusting the other scripts in a follow up. We don't have that many scripts in the first place so the ordering itself shouldn't be hard to figure out.

What we need to consider is how the structure of the scripts dir should look like though. Currently there are scripts on root level, in a message subdirectory and in a grub subdirectory introduced in #39. I suppose we should only "enumerate" the scripts which make it into the final Combustion script.

[jdob]

In this case, the XX_ was intentionally used because of the alphabetical sorting. We're gonna need the RPMs in place before the elemental register stuff kicks in.

But you're right that we need to go back through and evaluate all of them for this sort of thing. Danny and I were talking yesterday, I'm thinking something like 0X-5X are what we use, opening up 6X-8X for users (this will need to be documented, of course). That way the user has some room to play with their own custom scripts while also having hooks if they need to fit into a specific point in what we do.

[atanasdinov]

A couple of issues with the tests. Happy to approve after those are resolved, linter issues are fixed (using camelCase for variable names) and the /testdata/rpms dir is dropped.

[rdoxenham]

Just to ensure that we're all on the same page, I just pushed this (#38) as an example for how a local repo could be generated; the repo here is important as it can be simply added from Combustion:

zypper ar file:///dev/shm/combustion/config/repo airgap-repo
zypper --no-gpg-checks install -y --force-resolution --auto-agree-with-licenses <list of packages>

I know that @ipetrov117 is working on making that script more consumable via EIB but it's important that we're considering user-provided RPM's, along with a list of package names that can be pulled from SLE repo's as well as user-provided repo's. Therefore we need to think about this PR in the context of generating the entire repo, and installing an identified list of packages, rather than just rpm's placed into the /rpms directory.

[atanasdinov]

A final touch would be to reduce the duplicate setup code in tests with something similar to the following:

func setupRPMSourceDir(t *testing.T) (string, func()) {
	tmpDir, err := os.MkdirTemp("", "eib-RPM-")
	require.NoError(t, err)

	rpmSourceDir := filepath.Join(tmpDir, "rpms")
	err = os.Mkdir(rpmSourceDir, 0o755)
	require.NoError(t, err)

	file1Path := filepath.Join(rpmSourceDir, "rpm1.rpm")
	file1, err := os.Create(file1Path)
	require.NoError(t, err)

	file2Path := filepath.Join(rpmSourceDir, "rpm2.rpm")
	file2, err := os.Create(file2Path)
	require.NoError(t, err)

	return tmpDir, func() {
		assert.NoError(t, file1.Close())
		assert.NoError(t, file2.Close())
		assert.NoError(t, os.RemoveAll(tmpDir))
	}
}

You can then use this in the various different test functions with:

rpmSourceDir, teardown := setupRPMSourceDir(t)
defer teardown()

[dbw7]

A final touch would be to reduce the duplicate setup code in tests with something similar to the following:

func setupRPMSourceDir(t *testing.T) (string, func()) {
	tmpDir, err := os.MkdirTemp("", "eib-RPM-")
	require.NoError(t, err)

	rpmSourceDir := filepath.Join(tmpDir, "rpms")
	err = os.Mkdir(rpmSourceDir, 0o755)
	require.NoError(t, err)

	file1Path := filepath.Join(rpmSourceDir, "rpm1.rpm")
	file1, err := os.Create(file1Path)
	require.NoError(t, err)

	file2Path := filepath.Join(rpmSourceDir, "rpm2.rpm")
	file2, err := os.Create(file2Path)
	require.NoError(t, err)

	return tmpDir, func() {
		assert.NoError(t, file1.Close())
		assert.NoError(t, file2.Close())
		assert.NoError(t, os.RemoveAll(tmpDir))
	}
}

You can then use this in the various different test functions with:

rpmSourceDir, teardown := setupRPMSourceDir(t)
defer teardown()

Now this is awesome, thank you for writing this. I wasn't sure if it was a normal thing to make helper functions for tests so I avoided it, but I'm glad to see it's possible. Extra thanks as this allowed me to clean up the code significantly.