feat(ci): add windows builds to dev & pr workflows

[amdprophet]

Signed-off-by: Justin Kolberg [email protected]

Adds Windows builds to the Dev builds & PRs checks GitHub Actions CI workflows.

[andrzej-stencel]

👏 Impressive to see the engineering that went into this, but also scary to see how much more complicated the build process is becoming, given it was already quite complex. 😞

Probably the most important requirement is to be able to perform the build on a machine other than a GitHub Actions runner (for example a local laptop, or an EC2). Some documentation would probably be needed for this.

[perk-sumo]

Agreed with @astencel-sumo - document describing how to build it locally would be a very nice addition here.

Ideally this would work outside of Github Actions, eg. with bash script or sth.

[amdprophet]

@astencel-sumo @perk-sumo I've started throwing together some notes to document how to build locally on Windows. Should I put it in the otelcolbuilder README.md file or somewhere in the docs directory?

I'm working on getting make gotest to pass at the moment. I've come across a test that checks the permissions of a temporary directory created by the tests (https://github.com/SumoLogic/sumologic-otel-collector/blob/main/pkg/extension/sumologicextension/extension_test.go#L251). Temporary directory permissions differ between Linux & Windows. I'm wondering if we can remove the check and instead run chmod 700 against the temp directory.

[andrzej-stencel]

@astencel-sumo @perk-sumo I've started throwing together some notes to document how to build locally on Windows.

Can't thank you enough for your work @amdprophet 🙇

Should I put it in the otelcolbuilder README.md file or somewhere in the docs directory?

My first thought is CONTRIBUTING.md in the root directory, what do you think?

I'm working on getting make gotest to pass at the moment. I've come across a test that checks the permissions of a temporary directory created by the tests (https://github.com/SumoLogic/sumologic-otel-collector/blob/main/pkg/extension/sumologicextension/extension_test.go#L251). Temporary directory permissions differ between Linux & Windows. I'm wondering if we can remove the check and instead run chmod 700 against the temp directory.

Hm yeah it doesn't seem that this check makes any sense. It doesn't seem to be testing any functionality of the extension, it's reather an assertion on how the os.MkdirTmp() function works, which I don't think is relevant to anything. I support removing this check.

[amdprophet]

@astencel-sumo CONTRIBUTING.md makes sense to me. I've added some documentation to it; please let me know if you think it needs any changes.

[amdprophet]

I believe this is ready for review again. There are two tests that are failing intermittently with rawk8seventsreceiver but they don't seem to be Windows-specific as they're also occasionally failing in macOS.

The two tests are TestProcessEventE2E and TestStorage and the failure for both is due to LogRecordCount() returning 0 instead of the expected 1.

[andrzej-stencel]

Thanks a lot @amdprophet, this looks much much better. Following the instructions, I was able to build the binary on Windows and it seems to work perfectly fine. 👌 I might have just a few nits for the instructions.

Here's a silly question though: I just noticed there's an otelcol-sumo-windows_amd64 target in the otelcolbuilder/Makefile that seems to also build a Windows binary. What's the benefit of running the build on an actual Windows runner, if there is any?

[andrzej-stencel]

I don't think I would bother the user with installing the Windows Terminal. I mean, it's a good app that I use and would recommend to anyone, but it's orthogonal to the instructions on building the binary and it's an additional step, even if optional, which you could argue is even worse, because the user now needs to make a decision, which is the harder to make the more uninformed the user is. 🙂

I'd rather keep the steps as minimal as possible.

[andrzej-stencel]

I think I would assume the user has the repository already cloned, I would skip this step and straight to cd otelcolbuilder, the assumption being the user is already in the root of the repo. This is what is being done in the How to build steps below and I think it's enough.

[amdprophet]

Thanks a lot @amdprophet, this looks much much better. Following the instructions, I was able to build the binary on Windows and it seems to work perfectly fine. 👌 I might have just a few nits for the instructions.

Here's a silly question though: I just noticed there's an otelcol-sumo-windows_amd64 target in the otelcolbuilder/Makefile that seems to also build a Windows binary. What's the benefit of running the build on an actual Windows runner, if there is any?

The binary doesn't have to be built on a Windows runner but the MSI packaging will. Rather than setting up a complex pipeline that can pass binaries from one runner to another, I thought it would be best to run everything on a single Windows runner.

[andrzej-stencel]

The binary doesn't have to be built on a Windows runner but the MSI packaging will.

Oh I see, so we want to expand this in the future to things that are easier/more feasible on Windows OS (like building the installer). Thanks for the explanation 👍

[swiatekm]

@amdprophet We need to prevent the install script tests from running on Windows as well. Right now, unsurprisingly, they fail: https://github.com/SumoLogic/sumologic-otel-collector/actions/runs/3332408749/jobs/5522454895.

I think it's sufficient to add a build tag to the tests module: https://github.com/SumoLogic/sumologic-otel-collector/tree/main/pkg/scripts_test

[amdprophet]

@swiatekm-sumo I've already added build tags to prevent execution of those tests on Windows. It seems like this is due to the Makefile attempting to run the tests with sudo which doesn't exist on the Windows runner. I've used a variable called SUDO in the past which contains the name or path to sudo and then I overridde it to an empty string on platforms where there is no sudo. What do you think?

[amdprophet]

It seems that the env command is problematic on Windows as well https://github.com/SumoLogic/sumologic-otel-collector/blob/main/pkg/scripts_test/Makefile#L5. Perhaps conditional behaviour for that make target is a better choice.

[swiatekm]

chmod +x ./cmd/otelcol-sumo-windows_amd64.exe.exe

Looks like you've got an extraneous .exe somewhere in there.

[swiatekm]

LGTM

[amdprophet]

I believe the failing test just needs to be rerun. I saw intermittent failures of that test on both Windows & macOS.