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

Add Runner File Commands #684

Merged
merged 2 commits into from
Aug 28, 2020
Merged

Add Runner File Commands #684

merged 2 commits into from
Aug 28, 2020

Conversation

thboop
Copy link
Collaborator

@thboop thboop commented Aug 27, 2020

This PR cleans up the file command code in the runner.

@thboop thboop requested a review from ericsciple August 27, 2020 23:50
@thboop thboop marked this pull request as draft August 27, 2020 23:54
@thboop thboop marked this pull request as ready for review August 28, 2020 18:42
// Setup File Command Manager
var fileCommandManager = HostContext.CreateService<IFileCommandManager>();
// Container Action Handler will handle the conversion for Container Actions
var container = handlerData.ExecutionType == ActionExecutionType.Container ? null : ExecutionContext.Global.Container;
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's be consistent with existing bugs and add this as the host-path always.

container.MountVolumes.Add(new MountVolume(defaultWorkingDirectory, "/github/workspace"));

container.AddPathTranslateMapping(tempHomeDirectory, "/github/home");
container.AddPathTranslateMapping(tempWorkflowDirectory, "/github/workflow");
container.AddPathTranslateMapping(tempFileCommandDirectory, "/github/file_commands");
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit file-commands?

Copy link
Collaborator

Choose a reason for hiding this comment

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

doesnt matter really, temp file anyway

@@ -161,16 +161,21 @@ public async Task RunAsync(ActionRunStage stage)
Directory.CreateDirectory(tempHomeDirectory);
this.Environment["HOME"] = tempHomeDirectory;

var tempFileCommandDirectory = Path.Combine(tempDirectory, "_runner_file_commands");
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: _runner-file-commands?

@@ -13,11 +13,13 @@ public sealed class GitHubContext : DictionaryContextData, IEnvironmentContextDa
"actor",
"api_url",
"base_ref",
"env",
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we change to allow terminology (existing code above)

public interface IFileCommandExtension : IExtension
{
string ContextName { get; }
string FileName { get; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider FilePrefix

}
}

private bool TryDeleteFile(string path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider using IOUtil

@@ -239,6 +245,8 @@ public async Task RunAsync()

// Run the task.
await handler.RunAsync(Stage);
fileCommandManager.TryProcessFiles(ExecutionContext, ExecutionContext.Global.Container);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if the step fail? Do the file-commands still get processed?

For example, esp consider continue-on-error (script exits 1).

Copy link
Collaborator

Choose a reason for hiding this comment

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

May need try/finally. If we move into finally, need to make sure doesn't clobber original exception

Copy link
Collaborator

Choose a reason for hiding this comment

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

if it fails, set task result failed

@@ -145,6 +145,12 @@ public async Task RunAsync()
stepHost = containerStepHost;
}

// Setup File Command Manager
Copy link
Collaborator

Choose a reason for hiding this comment

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

check whether job extension step (initailize job) uses action runner

Copy link
Collaborator

Choose a reason for hiding this comment

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

may not matter if it doesnt set env/path

File.Create(newPath).Dispose();

var pathToSet = container != null ? container.TranslateToContainerPath(newPath) : newPath;
context.SetGitHubContext(fileCommand.ContextName, pathToSet);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: would be rad if more explicitly set in ActionRunner. Hard to mentally keep track where the global context is changed. Today it's only StepsRunner/CompositeActionRunner. May be drawback of plugin abstraction.

}
}

public void TryProcessFiles(IExecutionContext context, ContainerInfo container)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Do, do not try :)

@@ -56,6 +56,10 @@ public sealed class ExtensionManager : RunnerService, IExtensionManager
Add<T>(extensions, "GitHub.Runner.Worker.EndGroupCommandExtension, Runner.Worker");
Add<T>(extensions, "GitHub.Runner.Worker.EchoCommandExtension, Runner.Worker");
break;
case "GitHub.Runner.Worker.IFileCommandExtension":
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: personally don't like extension :P

nit: technically not "commands". Wonder if different naming would be more intuitive (less overloading). Thinking about .env file as analogy.

@thboop thboop merged commit 9976cb9 into main Aug 28, 2020
@thboop thboop deleted the thboop/AddRunnerFileCommands branch August 28, 2020 19:32
fabianishere pushed a commit to fabianishere/runner that referenced this pull request Sep 23, 2020
* Add File Runner Commands
AdamOlech pushed a commit to antmicro/runner that referenced this pull request Jan 28, 2021
* Add File Runner Commands
TingluoHuang pushed a commit that referenced this pull request Apr 21, 2021
* Add File Runner Commands
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.

None yet

2 participants