search

Automattic / hostmgr

A tool for managing macOS VM hosts
Mozilla Public License 2.0
8 stars 3 forks source link

Simplify buildkite job script generation #28

Closed crazytonyli closed 1 year ago

crazytonyli commented 1 year ago

Instead of having a list of environment variables to include in the buildkite script, we could look into the environment variables and pick up all buildkite ones (BUILDKITE*). If buildkite decides to add a new env var, we don't have to update our code now. One side effect I can think of is, if there are some buildkite env vars we don't want to include in the script, this PR will break that implicit feature.

Another small change is, escaping quotes in all environment variables. We've seen a related issue popped up recently, this change should prevent it from happening again.

I thought about using the env.txt buildkite API (shown in this doc) to implement both changes above—the API returns all buildkite envs which are properly "quote escaped". But it looks like a overkill. Please let me know what you think about this approach. Will it make the implementation simpler and more reliable?

crazytonyli commented 1 year ago

I thought I'll provide a little bit context for the new changes here: https://github.com/Automattic/hostmgr/pull/29 which is targeted to this PR branch has been merged, my original changes has been replaced by Jeremy's much nicer implementation. That's why the new diff you see here is very similar to the one in https://github.com/Automattic/hostmgr/pull/29.

AliSoftware commented 1 year ago

Finally, if you take a step back and look at the BuildkiteScriptGenerator as a whole, you'll realize that nothing from its code is specific to Buildkite: the logic of the class could be used to generate any sh or bash script

Writing this comment above made me remember: didn't we discuss that it would be a good idea to add a shebang for #!/bin/bash at the top of the generated script (which I think the current implementation doesn't, right?) to ensure compatibility and resilience against the host and VM using a different shell (and potentially incompatible with the generated script's syntax)?

Or was this postponed for a future subsequent PR refining this later?

crazytonyli commented 1 year ago

@AliSoftware it's postponed and tracked in https://github.com/Automattic/hostmgr/issues/30

mokagio commented 1 year ago

@crazytonyli I think it should be safe to dismiss @rynaardb's request for changes and go ahead and merge this PR by now?

crazytonyli commented 1 year ago

@mokagio Sounds good!