Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows support #144

Merged
merged 54 commits into from
Mar 10, 2022
Merged

Windows support #144

merged 54 commits into from
Mar 10, 2022

Conversation

lucaspin
Copy link
Contributor

@lucaspin lucaspin commented Mar 1, 2022

Adds initial Windows support for the agent. Also, to make things easier to review, this PR is only concerned with the implementation. Other things will be added in different PRs:

The main changes in this PR are explained below.

Docker compose executor support

docker-compose executor support will not be added in this PR, so we fail early on Executor.Prepare() if a docker-compose executor job is running on Windows. Also, we skip docker-compose executor unit tests in windows.

PTY support

Using a PTY to run commands isn't an option (see creack/pty#95). To address that limitation, after every command, we dump the environment, and use that environment dump to update our state of the environment.

File injection

  • File injection for shell executor does not use a temporary file anymore, as that's only needed for docker-compose executor.
  • Zebra uses either 0644 or 0600, so there is no real need for us to support file modes like +x, so that is not supported anymore.
  • File modes in Windows do not work like in UNIX-based systems. Windows manages file permissions through ACLs, which are not implemented in this PR. Instead, we rely on os.Chmod's behavior: 04xx creates a read-only file and 06xx creates a read-write file.

Stopping jobs

Since we don't have a PTY for windows, we don't have a parent process which we can kill and will kill all its child processes for us. We need a way to keep track of all processes that are started, so we could kill them all once the job finishes or is stopped. We do that using job objects. When the shell starts, we create a job object to which we assign each command's process. When the job finishes or is stopped, we close the job object handle, which will terminate all the processes associated with it.

Other changes

  • Shutdown hook path can specify a powershell script
  • We now use job.Executor.ExportEnvVars() to set the SEMAPHORE_JOB_RESULT environment variable. These changed the job output a little bit. All the changes in the E2Es are related to this.
  • We can't use hardcoded /tmp's anymore, we use os.TempDir() instead
  • New env.go to help managing environment variables
  • .env files used to expose environment variables now use a .env.{EpochNanos} name and are removed once the job is finished.

Local development

A new Vagrantfile with a provisioning script are added to help with local development and testing.

@lucaspin lucaspin requested a review from shiroyasha March 1, 2022 16:14
)

type Environment struct {
env map[string]string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's double-check if it is ok to keep this as a map to not introduce anything unexpected. One assumption that is broken here is that the order of the env vars is no longer guaranteed.

Does this matter?

It could while adding secrets. Let's take this example:

secret A:
  TOKEN=abc

secret B:
  TOKEN=foo

and a pipeline:

job:
  secrets:
    - name: A
    - name: B

Now, if we store the values in a key-value array [{TOKEN=abc}, {TOKEN=foo}] it is guaranteed that every time we run a job, the value of the TOKEN will be foo.

If we use maps, this guarantee is no longer true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I re-read it, it seems that I've missed that we are adding the entries to the map "in-order" they are defined.

Which makes my previous statement not true.
Either way, it is good to double-check.

Copy link
Contributor Author

@lucaspin lucaspin Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The env var names are ordered in Append() and ToFile() to help with testing, since it's hard to assert the output if we don't guarantee order.

However, those methods are only called after the environment is already created with CreateEnvironment().
The CreateEnvironment() method is the one that takes the []api.EnvVars and the []config.HostEnvVars and puts them into a map, one by one, respecting the order of those slices. So, if a env var name is duplicated, its value will be the value of the last env var from the host (if any), or the value of the last env var in the job request.

I added a test for it here

@shiroyasha
Copy link
Contributor

Looks good. 👏 🍻

I've left some minor comments above, nothing major that would stop the release.

@lucaspin lucaspin merged commit fc7e17f into master Mar 10, 2022
@lucaspin lucaspin deleted the windows-support branch March 10, 2022 11:30
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.

2 participants