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

Added ability to run Dockerfile.SUFFIX ContainerAction #1738

Merged
merged 8 commits into from
Apr 28, 2022

Conversation

nikola-jokic
Copy link
Contributor

@nikola-jokic nikola-jokic commented Mar 9, 2022

Closes #1416

This is an enhancement to the runner allowing it to run Dockerfile.SUFFIX files, which is allowed by Docker, but doesn't work in GitHub Actions

@nikola-jokic nikola-jokic requested a review from a team as a code owner March 9, 2022 12:22
@fhammerl fhammerl self-assigned this Mar 9, 2022
@nikola-jokic nikola-jokic requested a review from fhammerl March 11, 2022 14:23
@nikola-jokic nikola-jokic requested a review from fhammerl March 11, 2022 15:52
fhammerl
fhammerl previously approved these changes Mar 14, 2022
src/Runner.Worker/Handlers/ContainerActionHandler.cs Outdated Show resolved Hide resolved
@fhammerl fhammerl dismissed their stale review March 14, 2022 15:42

Consider L0s

@nikola-jokic nikola-jokic requested a review from fhammerl March 15, 2022 15:04
@nikola-jokic nikola-jokic requested a review from thboop March 24, 2022 14:30
@fhammerl
Copy link
Contributor

@thboop Do you think we should wrap this one in a FF?


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.

Copy link
Collaborator

@thboop thboop left a comment

Choose a reason for hiding this comment

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

lgtm

@thboop thboop merged commit 20b7e86 into actions:main Apr 28, 2022
@nikola-jokic nikola-jokic deleted the nikola-jokic/enhancement/1416 branch April 28, 2022 07:43
@thboop thboop mentioned this pull request Apr 29, 2022
thboop added a commit that referenced this pull request Apr 29, 2022
* Revert "Added ability to run Dockerfile.SUFFIX ContainerAction (#1738)"

20b7e86

* 291.1 release notes
cmmarslender pushed a commit to Chia-Network/runner that referenced this pull request May 6, 2022
* Added ability to run Dockerfile.SUFFIX ContainerAction

* Extracted IsDockerFile method

* reformatted, moved from index to Last()

* extracted IsDockerfile to DockerUtil with L0

* added check for IsDockerfile to account for docker://

* updated test to clearly show path/dockerfile:tag

* fail if Data.Image is not Dockerfile or docker://[image]
cmmarslender pushed a commit to Chia-Network/runner that referenced this pull request May 6, 2022
* Revert "Added ability to run Dockerfile.SUFFIX ContainerAction (actions#1738)"

20b7e86

* 291.1 release notes
ChristopherHX added a commit to ChristopherHX/runner.server that referenced this pull request May 11, 2022
thboop added a commit that referenced this pull request May 22, 2022
* Revert "Added ability to run Dockerfile.SUFFIX ContainerAction (#1738)"

20b7e86

* port release notes
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.

Bug when parsing runs.image value in action.yml
3 participants