fix: support docker create arguments from container.options (#1022) #1351

alex-savchuk · 2022-09-16T06:34:47Z

Important note: parsing of CLI options of docker is fetched from docker/cli sources explicitly - see docker_cli.go.
Please confirm it's acceptable.

Fixes #711
Fixes #1022

mergify · 2022-09-16T06:37:13Z

@alex-savchuk this pull request has failed checks 🛠

codecov · 2022-09-16T06:45:24Z

Codecov Report

Merging #1351 (0ea145d) into master (4f8da0a) will increase coverage by 6.58%.
The diff coverage is 75.38%.

@@            Coverage Diff             @@
##           master    #1351      +/-   ##
==========================================
+ Coverage   57.50%   64.09%   +6.58%     
==========================================
  Files          32       41       +9     
  Lines        4594     6380    +1786     
==========================================
+ Hits         2642     4089    +1447     
- Misses       1729     1996     +267     
- Partials      223      295      +72

Impacted Files	Coverage Δ
pkg/model/action.go	`0.00% <0.00%> (ø)`
pkg/model/step_result.go	`0.00% <ø> (ø)`
pkg/container/docker_run.go	`13.06% <12.32%> (+7.52%)`	⬆️
pkg/model/planner.go	`48.82% <25.00%> (-1.60%)`	⬇️
pkg/model/workflow.go	`50.97% <32.14%> (+0.05%)`	⬆️
pkg/container/docker_pull.go	`33.33% <33.33%> (ø)`
pkg/container/file_collector.go	`45.87% <45.87%> (ø)`
pkg/common/git/git.go	`50.00% <47.91%> (ø)`
pkg/container/docker_auth.go	`47.61% <50.00%> (+2.61%)`	⬆️
pkg/exprparser/interpreter.go	`73.37% <53.48%> (-0.02%)`	⬇️
... and 31 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mergify · 2022-09-16T19:16:21Z

@alex-savchuk this pull request has failed checks 🛠

mergify · 2022-09-16T19:21:49Z

@alex-savchuk this pull request has failed checks 🛠

ChristopherHX

I'm ok with copying this file from docker cli, if this allows act to have amost full docker options support. The opinion of the other maintainer matters, I can't merge anything without their consent.

pkg/container/docker_cli.go

mergify · 2022-09-17T16:07:29Z

@alex-savchuk this pull request has failed checks 🛠

mergify · 2022-09-17T16:48:27Z

@alex-savchuk this pull request has failed checks 🛠

mergify · 2022-09-17T17:17:13Z

@alex-savchuk this pull request has failed checks 🛠

mergify · 2022-09-17T17:47:26Z

@alex-savchuk this pull request has failed checks 🛠

mergify · 2022-09-17T18:42:40Z

@alex-savchuk this pull request has failed checks 🛠

alex-savchuk · 2022-09-18T19:45:54Z

It's a bit not clear for me what to do with codecov/project check.

I look at diff report at https://codecov.io/gh/nektos/act/compare/4f8da0a51cd44753f3e4ea0364877f6d2aa11f1d...9781bfbe18671a3529340150d12c3eb6c5e20c67/changes
but don't understand how my small changes result into this decreasing.

Maybe we need to switch HEAD of coverage report to some modern version ?
Or I need to add tests for logger.go/run_context.go in PR?

Upd: solved by adding tests of opts.go

mergify · 2022-09-19T02:17:56Z

@alex-savchuk this pull request is now in conflict 😩

mergify · 2022-09-19T09:07:46Z

@alex-savchuk this pull request has failed checks 🛠

KnisterPeter

Besides the license, this looks fine to me. Great addition.

pkg/container/docker_cli.go

mergify · 2022-09-20T20:17:51Z

@alex-savchuk this pull request has failed checks 🛠

mergify · 2022-09-20T23:04:05Z

@alex-savchuk this pull request has failed checks 🛠

ChristopherHX · 2022-09-24T10:00:37Z

but don't understand how my small changes result into this decreasing.

Your change isn't that small 1000 new lines of text without tests from the docker cli. I think you can fix the high coverage decrease by adding the test file of the added docker cli file https://github.com/docker/cli/blob/9ac8584acfd501c3f4da0e845e3a40ed15c85041/cli/command/container/opts_test.go.

I'm not approving yet, because I know that only the owner can merge a PR with a failed status check and I haven't tested it yet.

mergify · 2022-09-26T02:28:47Z

@alex-savchuk this pull request is now in conflict 😩

alex-savchuk · 2022-09-28T11:09:16Z

After merge #711, #1022 are expected to be fixed.
Also passing of docker options may help to resolve #676, #569, #1091

mergify · 2022-10-03T20:53:10Z

@alex-savchuk this pull request is now in conflict 😩

alex-savchuk · 2022-10-04T13:16:26Z

@act-maintainers Now we have coverage, tests etc. Could you please take a look and approve/merge if everything is ok? :)

ChristopherHX

Good work, this probably works better than the original actions/runner.
For the record, I tested with this workflow

on: push
jobs:
  _:
    container:
      image: ubuntu:latest
      options: --privileged --network=bridge
    steps:
      - run: echo "Hello World"

And inpected the docker container via docker inspect to see if the flags were applied.
Big PR's like this one seem to have very long review times to get 3 approvals

mergify · 2022-10-04T22:54:23Z

@alex-savchuk this pull request is now in conflict 😩

KnisterPeter

I really like it

)

cplee

Sorry for the delay - this was a large PR and took awhile to get through all the changes. Thanks for your contribution!

…ektos#1022) (nektos#1351)" This reverts commit 48188a6.

alex-savchuk requested a review from a team as a code owner September 16, 2022 06:34

pull-request-size bot added the size/XXL label Sep 16, 2022

mergify bot added the needs-work Extra attention is needed label Sep 16, 2022

alex-savchuk force-pushed the support-docker-create-args branch from 93f87ce to 1d0c067 Compare September 16, 2022 19:20

ChristopherHX reviewed Sep 17, 2022

View reviewed changes

pkg/container/docker_cli.go Show resolved Hide resolved

alex-savchuk force-pushed the support-docker-create-args branch from 16583b0 to baf949b Compare September 17, 2022 17:14

alex-savchuk force-pushed the support-docker-create-args branch from baf949b to 73242e6 Compare September 17, 2022 17:44

alex-savchuk force-pushed the support-docker-create-args branch from 73242e6 to 9781bfb Compare September 17, 2022 18:35

mergify bot added the conflict PR has conflicts label Sep 19, 2022

alex-savchuk force-pushed the support-docker-create-args branch from 9781bfb to 1a9b3c9 Compare September 19, 2022 08:57

mergify bot removed the conflict PR has conflicts label Sep 19, 2022

KnisterPeter suggested changes Sep 20, 2022

View reviewed changes

pkg/container/docker_cli.go Outdated Show resolved Hide resolved

alex-savchuk force-pushed the support-docker-create-args branch from f931a27 to ee89687 Compare September 20, 2022 22:54

KnisterPeter previously approved these changes Sep 21, 2022

View reviewed changes

mergify bot added the conflict PR has conflicts label Oct 3, 2022

alex-savchuk force-pushed the support-docker-create-args branch from b5fe3b3 to 097f894 Compare October 4, 2022 13:14

mergify bot removed the conflict PR has conflicts label Oct 4, 2022

ChristopherHX previously approved these changes Oct 4, 2022

View reviewed changes

mergify bot added the conflict PR has conflicts label Oct 4, 2022

KnisterPeter previously approved these changes Oct 5, 2022

View reviewed changes

alex-savchuk added 6 commits October 5, 2022 09:29

fix: support docker create arguments from container.options (nektos#1022

95dc69f

)

fix processing of errors, add verbose logging, fix test

6711e94

disable linter for code copied from docker/cli

9625c78

fix all linter issues

fcd5a96

Add license info

5d8ab29

Add opts_test.go from docker/cli and required testdata

c7870dd

alex-savchuk dismissed stale reviews from KnisterPeter and ChristopherHX via c7870dd October 5, 2022 06:31

alex-savchuk force-pushed the support-docker-create-args branch from 097f894 to c7870dd Compare October 5, 2022 06:31

KnisterPeter approved these changes Oct 5, 2022

View reviewed changes

mergify bot removed the conflict PR has conflicts label Oct 5, 2022

ChristopherHX approved these changes Oct 5, 2022

View reviewed changes

cplee approved these changes Oct 6, 2022

View reviewed changes

Merge branch 'master' into support-docker-create-args

0ea145d

mergify bot merged commit 48188a6 into nektos:master Oct 6, 2022

KnisterPeter added a commit to xing/act that referenced this pull request Oct 7, 2022

Revert "fix: support docker create arguments from container.options (n…

bc25f6a

…ektos#1022) (nektos#1351)" This reverts commit 48188a6.

KnisterPeter mentioned this pull request Oct 7, 2022

Revert "fix: support docker create arguments from container.options (… xing/act#77

Closed

github-actions bot pushed a commit to xing/act that referenced this pull request Oct 7, 2022

Revert "fix: support docker create arguments from container.options (n…

4439fee

…ektos#1022) (nektos#1351)" This reverts commit 48188a6.

KnisterPeter mentioned this pull request Oct 7, 2022

test: add test for networking setup in act #1375

Merged

github-actions bot pushed a commit to xing/act that referenced this pull request Oct 8, 2022

Revert "fix: support docker create arguments from container.options (n…

1aed7ad

…ektos#1022) (nektos#1351)" This reverts commit 48188a6.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support docker create arguments from container.options (#1022) #1351

fix: support docker create arguments from container.options (#1022) #1351

alex-savchuk commented Sep 16, 2022 •

edited by catthehacker

Loading

mergify bot commented Sep 16, 2022

codecov bot commented Sep 16, 2022 •

edited

Loading

mergify bot commented Sep 16, 2022

mergify bot commented Sep 16, 2022

ChristopherHX left a comment

mergify bot commented Sep 17, 2022

mergify bot commented Sep 17, 2022

mergify bot commented Sep 17, 2022

mergify bot commented Sep 17, 2022

mergify bot commented Sep 17, 2022

alex-savchuk commented Sep 18, 2022 •

edited

Loading

mergify bot commented Sep 19, 2022

mergify bot commented Sep 19, 2022

KnisterPeter left a comment

mergify bot commented Sep 20, 2022

mergify bot commented Sep 20, 2022

ChristopherHX commented Sep 24, 2022

mergify bot commented Sep 26, 2022

alex-savchuk commented Sep 28, 2022

mergify bot commented Oct 3, 2022

alex-savchuk commented Oct 4, 2022

ChristopherHX left a comment

mergify bot commented Oct 4, 2022

KnisterPeter left a comment

cplee left a comment

fix: support docker create arguments from container.options (#1022) #1351

fix: support docker create arguments from container.options (#1022) #1351

Conversation

alex-savchuk commented Sep 16, 2022 • edited by catthehacker Loading

mergify bot commented Sep 16, 2022

codecov bot commented Sep 16, 2022 • edited Loading

Codecov Report

mergify bot commented Sep 16, 2022

mergify bot commented Sep 16, 2022

ChristopherHX left a comment

Choose a reason for hiding this comment

mergify bot commented Sep 17, 2022

mergify bot commented Sep 17, 2022

mergify bot commented Sep 17, 2022

mergify bot commented Sep 17, 2022

mergify bot commented Sep 17, 2022

alex-savchuk commented Sep 18, 2022 • edited Loading

mergify bot commented Sep 19, 2022

mergify bot commented Sep 19, 2022

KnisterPeter left a comment

Choose a reason for hiding this comment

mergify bot commented Sep 20, 2022

mergify bot commented Sep 20, 2022

ChristopherHX commented Sep 24, 2022

mergify bot commented Sep 26, 2022

alex-savchuk commented Sep 28, 2022

mergify bot commented Oct 3, 2022

alex-savchuk commented Oct 4, 2022

ChristopherHX left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 4, 2022

KnisterPeter left a comment

Choose a reason for hiding this comment

cplee left a comment

Choose a reason for hiding this comment

alex-savchuk commented Sep 16, 2022 •

edited by catthehacker

Loading

codecov bot commented Sep 16, 2022 •

edited

Loading

alex-savchuk commented Sep 18, 2022 •

edited

Loading