fix: copy git tracked files (#912) by jsoref · Pull Request #949 · nektos/act

jsoref · 2022-01-02T06:22:01Z

So, this is almost certainly not the right fix for #912, but for git oriented projects it does something which would probably be good enough for me anyway. -- Note: I'm not a go dev, so the style here is probably very wrong, it's just an attempt at playing with things. I'm sure I could find the proper go-git function to ask "are you tracked", but I decided to start with the simpler "go ls-files" approach.

Before

% act
[What do I have/Introspection] 🚀  Start image=catthehacker/ubuntu:act-latest
[What do I have/Introspection]   🐳  docker pull image=catthehacker/ubuntu:act-latest platform= username= forcePull=false
[What do I have/Introspection]   🐳  docker create image=catthehacker/ubuntu:act-latest platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[What do I have/Introspection]   🐳  docker run image=catthehacker/ubuntu:act-latest platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[What do I have/Introspection]   🐳  docker exec cmd=[mkdir -m 0777 -p /var/run/act] user=root workdir=
[What do I have/Introspection]   🐳  docker cp src=/Users/jsoref/code/nektos/ignored/. dst=/Users/jsoref/code/nektos/ignored
[What do I have/Introspection]   🐳  docker exec cmd=[mkdir -p /Users/jsoref/code/nektos/ignored] user= workdir=
[What do I have/Introspection] ⭐  Run checkout
[What do I have/Introspection]   ✅  Success - checkout
[What do I have/Introspection] ⭐  Run introspect
[What do I have/Introspection]   🐳  docker exec cmd=[bash --noprofile --norc -e -o pipefail /Users/jsoref/code/nektos/ignored/workflow/1] user= workdir=
| total 4
| drwxr-xr-x 2 root root 4096 Jan  2 06:15 workflow
[What do I have/Introspection]   ✅  Success - introspect

After

% ../act/dist/local/act
[What do I have/Introspection] 🚀  Start image=catthehacker/ubuntu:act-latest
[What do I have/Introspection]   🐳  docker pull image=catthehacker/ubuntu:act-latest platform= username= forcePull=false
[What do I have/Introspection]   🐳  docker create image=catthehacker/ubuntu:act-latest platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[What do I have/Introspection]   🐳  docker run image=catthehacker/ubuntu:act-latest platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[What do I have/Introspection]   🐳  docker exec cmd=[mkdir -m 0777 -p /var/run/act] user=root workdir=
[What do I have/Introspection]   🐳  docker cp src=/Users/jsoref/code/nektos/ignored/. dst=/Users/jsoref/code/nektos/ignored
[What do I have/Introspection]   🐳  docker exec cmd=[mkdir -p /Users/jsoref/code/nektos/ignored] user= workdir=
[What do I have/Introspection] ⭐  Run checkout
[What do I have/Introspection]   ✅  Success - checkout
[What do I have/Introspection] ⭐  Run introspect
[What do I have/Introspection]   🐳  docker exec cmd=[bash --noprofile --norc -e -o pipefail /var/run/act/workflow/1] user= workdir=
| total 0
| -rw-r--r-- 1 502 dialout 0 Jan  2 05:49 README.md
[What do I have/Introspection]   ✅  Success - introspect

mergify · 2022-01-02T06:24:40Z

@jsoref this pull request has failed checks 🛠

codecov · 2022-01-02T06:27:18Z

Codecov Report

Merging #949 (c8ccf20) into master (0f04942) will increase coverage by 7.76%.
The diff coverage is 58.95%.

@@            Coverage Diff             @@
##           master     #949      +/-   ##
==========================================
+ Coverage   49.27%   57.03%   +7.76%     
==========================================
  Files          23       32       +9     
  Lines        2401     4632    +2231     
==========================================
+ Hits         1183     2642    +1459     
- Misses       1090     1767     +677     
- Partials      128      223      +95

Impacted Files	Coverage Δ
pkg/common/executor.go	`46.49% <0.00%> (+1.61%)`	⬆️
pkg/common/job_error.go	`0.00% <0.00%> (ø)`
pkg/common/outbound_ip.go	`0.00% <0.00%> (ø)`
pkg/common/testflag.go	`0.00% <0.00%> (ø)`
pkg/container/docker_volume.go	`0.00% <0.00%> (ø)`
pkg/model/action.go	`0.00% <0.00%> (ø)`
pkg/model/github_context.go	`79.51% <ø> (ø)`
pkg/model/step_result.go	`0.00% <0.00%> (ø)`
pkg/container/docker_run.go	`5.19% <9.41%> (+3.26%)`	⬆️
pkg/common/git.go	`49.82% <31.81%> (-9.97%)`	⬇️
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e6cddf...c8ccf20. Read the comment docs.

github-actions · 2022-02-02T00:06:58Z

PR is stale and will be closed in 14 days unless there is new activity

ChristopherHX

Thanks, but I cannot approve this yet. Waiting for other maintainers to have a look.

ChristopherHX · 2022-02-02T15:30:57Z

-				linkName, err = os.Readlink(file)
+				_, err := os.Readlink(file)


Althought this change made the linter happy, it is a bad idea to skip assigning linkName.
var err error before linkName, err = os.Readlink(file), would still assign linkName.
Not assigning linkName here, breaks the whole symlink handling of act and all symlinks ends up pointing to itself.

[main.yml/test] 🐳 docker exec cmd=[bash --noprofile --norc -e -o pipefail /var/run/act/workflow/1] user= workdir= | total 14488 | drwxr-xr-x 3 root root 4096 Feb 2 15:37 . | drwxr-xr-x 3 root root 4096 Feb 2 15:37 .. | drwxr-xr-x 3 root root 4096 Feb 2 15:37 .github | -rwxrwxrwx 1 1000 1000 14819328 Feb 2 15:35 act | -rwxrwxrwx 1 1000 1000 5 Feb 2 15:37 atc | lrwxrwxrwx 1 1000 1000 8 Feb 2 15:37 main.yml -> main.yml

Symlink main.yml does actually point to .github/workflows/main.yml and not to itself.

No CI test detected that defect, we need better tests

I definitely wasn't pleased about the linter nor that I couldn't figure out the need.

Will see about the right change.

ChristopherHX · 2022-02-02T15:42:43Z

+			return err
+		}
+		if useGitIgnore {
+			if tryGitIgnore() != nil {


What if tryGitIgnore has a partial failure?
e.g. only one file failes, then you may have duplicated entries, since you just continue with the old codepath.

My hope was to abandon what I had, but thinking about it, I suspect I need to actually reset my stash and that I haven't done that yet.

ChristopherHX · 2022-02-02T15:58:08Z

+					walker := object.NewTreeWalker(tree, true, nil)
+
+					for {
+						var name string
+						name, _, err = walker.Next()


So this change no longer copies untracked files at all. I would expect they are still being copied if not ignored.
I use git ls-files -s -o --exclude-standard in my own client, no idea what go-git has for this functionality.

-s provide file mode bits and include staged changes ( only these file get copied by this PR )

-o provide untracked files

--exclude-standard remove ignored untracked files from the output.

Oh I forget to write, you have to be very careful if the repository has submodules. My own client need to do this recursively for all submodule objects, I forget this once and ran into my own bug.

I try very hard to stay very far away from submodules. Indeed, for this, I won't be able to...

I'll try to look tonight, maybe...

KnisterPeter · 2022-02-02T16:14:17Z

First of all thank you 😃

I find it hard to review currently, as we don't have tests (at least not much) for that.
I think it might really be good to separate your feature into it's own file with a function which decides if a file should be copied or not.
Then it would be quite easy to add test for that.

jsoref · 2022-02-02T16:41:55Z

That seems reasonable. If you have any preferences about naming, please feel free to provide it before I start.

I'm not a big go dev, but so far, the ability to refactor the code here makes me fairly confident that moving it into a distinct file wouldn't be a big deal.

KnisterPeter · 2022-02-02T16:47:29Z

That seems reasonable. If you have any preferences about naming, please feel free to provide it before I start.

I have no preference, just give it a start.

I'm not a big go dev, but so far, the ability to refactor the code here makes me fairly confident that moving it into a distinct file wouldn't be a big deal.

It should be straight forward if you stay in the folder (same package).

github-actions · 2022-03-06T00:08:53Z

PR is stale and will be closed in 14 days unless there is new activity

jsoref · 2022-03-06T00:27:12Z

Maybe i'll look again this week...

jsoref requested a review from a team as a code owner January 2, 2022 06:22

pull-request-size Bot added the size/M label Jan 2, 2022

mergify Bot added the needs-work Extra attention is needed label Jan 2, 2022

github-actions Bot added the stale label Feb 2, 2022

fix: copy git tracked files (nektos#912)

c8ccf20

jsoref force-pushed the issue-912 branch from 6bb7870 to c8ccf20 Compare February 2, 2022 03:02

pull-request-size Bot added size/L and removed size/M labels Feb 2, 2022

mergify Bot removed the needs-work Extra attention is needed label Feb 2, 2022

ChristopherHX requested changes Feb 2, 2022

View reviewed changes

mergify Bot requested a review from a team February 2, 2022 16:03

github-actions Bot removed the stale label Feb 3, 2022

github-actions Bot added the stale label Mar 6, 2022

github-actions Bot removed the stale label Mar 7, 2022

ChristopherHX mentioned this pull request Mar 24, 2022

fix: copy ignored tracked files #1074

Merged

mergify Bot closed this in #1074 Apr 4, 2022

		linkName, err = os.Readlink(file)
		_, err := os.Readlink(file)

Uh oh!

Conversation

jsoref commented Jan 2, 2022

Before

After

Uh oh!

mergify Bot commented Jan 2, 2022

Uh oh!

codecov Bot commented Jan 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Feb 2, 2022

Uh oh!

ChristopherHX left a comment

Choose a reason for hiding this comment

Uh oh!

ChristopherHX Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

jsoref Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

ChristopherHX Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

jsoref Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

ChristopherHX Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

ChristopherHX Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

jsoref Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

KnisterPeter commented Feb 2, 2022

Uh oh!

jsoref commented Feb 2, 2022

Uh oh!

KnisterPeter commented Feb 2, 2022

Uh oh!

github-actions Bot commented Mar 6, 2022

Uh oh!

jsoref commented Mar 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jan 2, 2022 •

edited

Loading