Skip to content

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Nov 16, 2022

I wanted to check if the agent had IMAGE_ID =~ ^xcode and run brew install xcodes if so in a pre-commit hook, but it turns out that env var is not available to the scripts.


I tried to add tests for this change but run into a limitation with the setup.

I started by duplicating the test from BuildkiteScriptBuilderTests that verifies that all BUILDKITE_* values are copied and modified it to use IMAGE_ID from a new .env file:

func testThatPullRequestEnvironmentVariableIsImported() throws {
let env = try DotEnv.read(path: codeQuoteEnvironmentPath)
scriptBuilder.copyEnvironmentVariables(prefixedBy: "BUILDKITE_", from: readLines(from: env))
XCTAssertEqual(
BuildkiteScriptBuilder.Value(wrapping: "19136"),
scriptBuilder.environmentVariables["BUILDKITE_PULL_REQUEST"]
)
}

I was surprised to see it passed. I then realized that test verifies the ability of the method to copy all env vars matching the given pattern from the given file. It makes no assertion on whether we do copy BUILDKITE_ env vars at runtime. The component responsible for that behavior is GenerateBuildkiteJobScript from the hostmgr executable target.

I don't know how to test an executable target. As a matter of fact, we do have a draft of test for hostmgr which does not run because not configured in Package.swift, SyncVMImagesCommandTests.

➜ swift test | grep SyncVMImagesCommandTests | wc -l
Compiling plugin GenerateManualPlugin...
Building for debugging...
Build complete! (0.30s)
0

# conversely, with a test that does run
➜ swift test | grep BuildkiteScriptBuilderTests | wc -l
Compiling plugin GenerateManualPlugin...
Building for debugging...
Build complete! (0.31s)
26

I have a "solution" in mind, which I'll propose in a dedicated issue. But for the time being, this change does the job of forwarding IMAGE_ID.

@crazytonyli
Copy link
Contributor

I'm wondering if you need IMAGE_ID for your purpose? Maybe you can check if the pipeline is running on a Mac? Like

[[ $(uname) == 'Darwin' ]] && brew install xcodes

@jkmassel
Copy link
Contributor

I don't know how to test an executable target

So..mostly you can't, which is why I made the libhostmgr package, from which things are easy to test – IMHO ideally the hostmgr tool would just be the CLI bits wrapping the actual functionality in libhostmgr.

WDYT?

@mokagio
Copy link
Contributor Author

mokagio commented Nov 19, 2022

@jkmassel

So..mostly you can't, which is why I made the libhostmgr package, from which things are easy to test – IMHO ideally the hostmgr tool would just be the CLI bits wrapping the actual functionality in libhostmgr.

WDYT

Yeah, that setup seems like a good approach to build a CLI. Functional, testable, fat core; imperative, untested, thin shell. I made a suggestion in #48 about it.

@mokagio
Copy link
Contributor Author

mokagio commented Nov 19, 2022

With xcodes being part of the tools available in the CI images starting 14.1, h/t @crazytonyli , the need for this is purely "just in case, for the future".

Closing.

@mokagio mokagio closed this Nov 19, 2022
@mokagio mokagio deleted the mokagio/forward-IMAGE_ID branch November 19, 2022 04:16
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