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.

Sign up for GitHub

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

Jump to bottom

Added ability to run Dockerfile.SUFFIX ContainerAction #1738

Merged
merged 8 commits into from
Apr 28, 2022
Merged

Added ability to run Dockerfile.SUFFIX ContainerAction #1738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8142f10
Added ability to run Dockerfile.SUFFIX ContainerAction
Mar 9, 2022
ec56106
Extracted IsDockerFile method
Mar 11, 2022
c4e0443
reformatted, moved from index to Last()
Mar 11, 2022
f98b197
extracted IsDockerfile to DockerUtil with L0
Mar 15, 2022
5f0c2f7
added check for IsDockerfile to account for docker://
Mar 22, 2022
735f6cb
updated test to clearly show path/dockerfile:tag
Mar 22, 2022
c560265
fail if Data.Image is not Dockerfile or docker://[image]
Mar 24, 2022
3f85311
Merge branch 'actions:main' into nikola-jokic/enhancement/1416
nikola-jokic Apr 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Runner.Worker/ActionManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1004,7 +1004,7 @@ private ActionSetupInfo PrepareRepositoryActionAsync(IExecutionContext execution
if (actionDefinitionData.Execution.ExecutionType == ActionExecutionType.Container)
{
var containerAction = actionDefinitionData.Execution as ContainerActionExecutionData;
if (containerAction.Image.EndsWith("Dockerfile") || containerAction.Image.EndsWith("dockerfile"))
if (DockerUtil.IsDockerfile(containerAction.Image))
nikola-jokic marked this conversation as resolved.
Show resolved Hide resolved
{
var dockerFileFullPath = Path.Combine(actionEntryDirectory, containerAction.Image);
executionContext.Debug($"Dockerfile for action: '{dockerFileFullPath}'.");
Expand Down
13 changes: 12 additions & 1 deletion src/Runner.Worker/Container/DockerUtil.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;

namespace GitHub.Runner.Worker.Container
Expand All @@ -17,7 +18,7 @@ public static List<PortMapping> ParseDockerPort(IList<string> portMappingLines)
string pattern = $"^(?<{targetPort}>\\d+)/(?<{proto}>\\w+) -> (?<{host}>.+):(?<{hostPort}>\\d+)$";

List<PortMapping> portMappings = new List<PortMapping>();
foreach(var line in portMappingLines)
foreach (var line in portMappingLines)
{
Match m = Regex.Match(line, pattern, RegexOptions.None, TimeSpan.FromSeconds(1));
if (m.Success)
Expand Down Expand Up @@ -61,5 +62,15 @@ public static string ParseRegistryHostnameFromImageName(string name)
}
return "";
}

public static bool IsDockerfile(string image)
{
if (image.StartsWith("docker://", StringComparison.OrdinalIgnoreCase))
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 keep this if, depending on how IsDockerFile is called, we may end up checking if the image starts with docker:// multiple times.

But as a util method, I think IsDockerFile should also do the check itself so it's not reliant on being called in an if/else that has checked docker:// already.

{
return false;
}
var imageWithoutPath = image.Split('/').Last();
nikola-jokic marked this conversation as resolved.
Show resolved Hide resolved
return imageWithoutPath.StartsWith("Dockerfile.") || imageWithoutPath.StartsWith("dockerfile.") || imageWithoutPath.EndsWith("Dockerfile") || imageWithoutPath.EndsWith("dockerfile");
}
}
}
7 changes: 5 additions & 2 deletions src/Runner.Worker/Handlers/ContainerActionHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,8 @@ public async Task RunAsync(ActionRunStage stage)
{
Data.Image = Data.Image.Substring("docker://".Length);
}
else if (Data.Image.EndsWith("Dockerfile") || Data.Image.EndsWith("dockerfile"))
else if (DockerUtil.IsDockerfile(Data.Image))
{
// ensure docker file exist
var dockerFile = Path.Combine(ActionDirectory, Data.Image);
nikola-jokic marked this conversation as resolved.
Show resolved Hide resolved
ArgUtil.File(dockerFile, nameof(Data.Image));

Expand All @@ -68,6 +67,10 @@ public async Task RunAsync(ActionRunStage stage)

Data.Image = imageName;
}
else
nikola-jokic marked this conversation as resolved.
Show resolved Hide resolved
{
throw new InvalidOperationException($"'{Data.Image}' should be either '[path]/Dockerfile' or 'docker://image[:tag]'.");
}

string type = Action.Type == Pipelines.ActionSourceType.Repository ? "Dockerfile" : "DockerHub";
// Set extra telemetry base on the current context.
Expand Down
43 changes: 43 additions & 0 deletions src/Test/L0/Container/DockerUtilL0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,5 +144,48 @@ public void ParseRegistryHostnameFromImageName(string input, string expected)
var actual = DockerUtil.ParseRegistryHostnameFromImageName(input);
Assert.Equal(expected, actual);
}

[Theory]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]
[InlineData("dockerhub/repo", false)]
[InlineData("debian:latest", false)]
[InlineData("something/dockerfileimage", false)]
[InlineData("ghcr.io/docker/dockerfile", true)] // should be false but might break the current workflows
[InlineData("Dockerfile", true)]
[InlineData("Dockerfile.", true)]
[InlineData(".Dockerfile", true)]
[InlineData(".Dockerfile.", false)]
[InlineData("dockerfile", true)]
[InlineData("dockerfile.", true)]
fhammerl marked this conversation as resolved.
Show resolved Hide resolved
[InlineData(".dockerfile", true)]
[InlineData(".dockerfile.", false)]
[InlineData("Dockerfile.test", true)]
[InlineData("test.Dockerfile", true)]
[InlineData("docker/dockerfile:latest", false)]
[InlineData("/some/path/dockerfile:latest", false)]
[InlineData("dockerfile:latest", false)]
[InlineData("Dockerfile:latest", false)]
[InlineData("dockerfile-latest", false)]
[InlineData("Dockerfile-latest", false)]
[InlineData("dockerfile.latest", true)]
[InlineData("Dockerfile.latest", true)]
[InlineData("../dockerfile/dockerfileone", false)]
[InlineData("../Dockerfile/dockerfileone", false)]
[InlineData("../dockerfile.test", true)]
[InlineData("../Dockerfile.test", true)]
[InlineData("./dockerfile/image", false)]
[InlineData("./Dockerfile/image", false)]
[InlineData("example/Dockerfile.test", true)]
[InlineData("./example/Dockerfile.test", true)]
[InlineData("example/test.dockerfile", true)]
[InlineData("./example/test.dockerfile", true)]
[InlineData("docker://Dockerfile", false)]
[InlineData("docker://ubuntu:latest", false)]
public void IsDockerfile(string input, bool expected)
{
var actual = DockerUtil.IsDockerfile(input);
Assert.Equal(expected, actual);
}
}
}