docker-lock tries to read directories with matching names as dockerfiles

[andoks]

docker lock generate seems to want to try to parse directories as if they are dockerfiles (this does not seems to happen for docker-compose entries).

E.g

mkdir test
cd test
mkdir Dockerfile
docker lock generate

This is particularly a problem when using --dockerfile-globs, as it is not possible to specify that the glob should only match files (AFAIK)

[michaelperel]

@andoks I merged a change to be more strict about checking directories and files that exist. Could you give it a try and let me know if those more strict changes give you any unforeseen headaches before I cut a new release?

[andoks]

@michaelperel: thanks!

I tested a bit, and I noticed that when there is a directory called "Dockerfile" with a docker-file inside it, I get the following error 'Dockerfile' was collected but is a directory rather than a file, and no docker-lock file is generated:

mkdir test
cd test
go mod init test
go get github.com/safe-waters/docker-lock/cmd/docker-lock@b32f5b3b69da8857ca8f0122c5ecfd9b6c5fa158
mkdir Dockerfile
echo "from hello-world" > Dockerfile/Dockerfile
go run github.com/safe-waters/docker-lock/cmd/docker-lock lock generate --dockerfile-recursive

[michaelperel]

@andoks I have fixed this issue. Would you be able to run it once more on your codebase and make sure there are no other headaches? Thanks :)

[andoks]

@michaelperel: Still get the error "'dockerfile' is a directory rather than a file" when doing the following:

mkdir test-directories-called-Dockerfile-3
cd test-directories-called-Dockerfile-3
go mod init test
mkdir dockerfile
echo "FROM hello-world" >> dockerfile/test-image.dockerfile
go get github.com/safe-waters/docker-lock/cmd/docker-lock@b64d62793ed26db19ea12e62fd097a47980db11a
go run github.com/safe-waters/docker-lock/cmd/docker-lock lock generate --dockerfile-recursive --dockerfile-globs '*dockerfile'

[michaelperel]

@andoks In your specific case, your glob pattern does not pick up the file you think it does, but instead only matches the directory.

Instead, your command should be:

docker lock generate --dockerfile-recursive --dockerfile-globs '**/*dockerfile'

This goes back to the discussion in #90 -

I think the case you want is "recursively search all folders, which match the filename with the regex *dockerfile". Currently, recursively searching matches a bunch of default names mentioned in the README, but a flag could be added to match a regex.

[michaelperel]

@andoks It is unfortunate that there are differences between specific language implementation's of regexes and globs, and since docker-lock is written in go, it follows go's conventions. Does the glob pattern provided cover your use case?

[andoks]

In your specific case, your glob pattern does not pick up the file you think it does, but instead only matches the directory. ...

Oh, yeah, I though that the flag was used to pass essentially what would be a filter regex (which is why I also passed --dockerfile-recursive, which I then though would trigger recursive walking through all files). That's my bad sorry!

I think the case you want is "recursively search all folders, which match the filename with the regex *dockerfile". Currently, recursively searching matches a bunch of default names mentioned in the README, but a flag could be added to match a regex.

This was indeed what I thought I did with the command yeah. But I think the glob flag is okay, but I misunderstood how it is working. If it is an easy mistake to do for others I would not know. On my part I do not think I need the regex support per se, since I now think I understand how I am supposed to use the globs flags.

... Does the glob pattern provided cover your use case?

Yes, I think so.

However I still get the following error when testing: 'some_dir/dockerfile' is a directory rather than a file. Would it be Ok to unchoke these errors when using globs too?

Test

mkdir -pv test-directories-called-Dockerfile-4
cd test-directories-called-Dockerfile-4
go mod init test
go get github.com/safe-waters/docker-lock/cmd/docker-lock@b64d62793ed26db19ea12e62fd097a47980db11a
mkdir -pv some_dir/dockerfile
echo 'FROM hello-world' > some_dir/dockerfile/hello.dockerfile
go run github.com/safe-waters/docker-lock/cmd/docker-lock lock generate --dockerfile-globs '**/*dockerfile'

[michaelperel]

@andoks In your last command, the glob pattern

**/*dockerfile

means match all paths that are 1 directory down with the pattern *dockerfile. So in this case, you actually are matching the directory some_dir/dockerfile, not the file, some_dir/dockerfile/hello.dockerfile.

If you want to go 2 directories down like in your example, you would have to use the pattern

**/**/*dockerfile

Unfortunately, ** does not mean match all directories recursively.

If docker-lock did not choke on this, you would think that your glob pattern was correct, which is why it currently chokes on it.

I wish that go's glob patterns had a way to recursively match all directories with **, but unfortunately, I haven't been able to find a glob pattern that means "recursively search all directories".

In projects that I have worked on, typically I know where the Dockerfiles are, so I just specify multiple glob patterns to the correct directories. For example:

docker lock generate --dockerfile-globs 'some_dir/**/*dockerfile','some_dir2/**/*dockerfile'

In your project, is it the case that you don't know where all the Dockerfiles are/they are not organized in a way? I think the recursive regex solution would solve cases like that.

[cicorias]

did not know that:

I wish that go's glob patterns had a way to recursively match all directories with **, but unfortunately, I haven't been able to find a glob pattern that means "recursively search all directories".

[andoks]

@michaelperel you are indeed right. I thought the globs worked similar to shell globs. Looking at the docs for filepath glob syntax, ** does not have any special meaning at all, and is equivalent to just * AFAICS (assuming I got the right package, at a glance it seemed like that is what is used in collector.go).

Would it be possible to replace the implementation used for globbing with something that support more powerful globs (e.g https://pkg.go.dev/github.com/gobwas/glob) or would that be considered a too much of a breaking change? If the only blocker is the amount of work, I could take a look at doing a PR.

If docker-lock did not choke on this, you would think that your glob pattern was correct, which is why it currently chokes on it.

I agree it is good it chokes on this the way globbing currently works. 👍

I wish that go's glob patterns had a way to recursively match all directories with **, but unfortunately, I haven't been able to find a glob pattern that means "recursively search all directories".

From the docs it does not look like it exist in go's path/filepath at all to me.

In your project, is it the case that you don't know where all the Dockerfiles are/they are not organized in a way?

Right now it is yes, so I can make do with listing all subdirs that have dockerfiles and compose files in them. But I wanted to also ensure that any new recipes created also were picked up without having to remember (or know of) a top-level CI config that needs to be updated. I know from experience there is a chance I will forget it myself, and if not me, then someone else on the team. But I am usually consistent in naming these files, so IMO there is a relative greater chance for that approach to work on my team. That was my motivation anyhow.

I think the recursive regex solution would solve cases like that.

It probably would, as long as 1) the regex filter is applied to all possible subpaths from CWD and 2) matching directories are ignored but recursed into. An alternative would be to use a more powerful glob library, showing an example in the help text for the glob flags increasing the chances for making the user understand that it is a glob not a regex, and stop erroring on matching directories (perhaps logging matching dirs to stdout, or add a -v flag to increase the log level to show these dirs)

[michaelperel]

@andoks I am open to using a 3rd party package, but I was thinking that it could complicate things if people expect things to work as go does.

Given our recent discussion, since people can use the find command, I think it may make sense to leave it as it is and if people want more powerful globs, they can use regular shell commands.

In light of this, I actually think it is not worth implementing the regex approach or a more powerful glob approach.

Do you agree, and is that ok for your use case?

[andoks]

@michaelperel I see your point re globs and regex. I suspect however that how the current globs works breaks the principle of least surprise for more users though. I am a go developer myself (arguably relatively fresh), but since I never have needed glob functionality in my programs so far, I was not aware of Gos particularities re glob syntax and support. And seeing as docker-lock is not particularly catering to go-developers, but docker users in general, even more so.

Regarding using find, and passing the result to --dockerfiles / --composefiles, this is a good approach, but IMO this too surprised me, that it used commmas' separation of files instead of spaces or newlines, and when I passed the raw output of find, it happily chugged along, but only parsed the first entry in the list. This however could be remedied quite effectively by mentioning in the flags --help text that the paths are comma-separated, possibly showing an example using find (although the example might be overfitting to unix platforms, maybe there is something equivalent for windows that can be switched for using an architecture or os build tag).

These are just my two cents, grain of salt and so forth...

For me in particular though, I have my usecases covered with the --dockerfiles/--composefiles + find + paste and the changes you have implemented so far 👍. Thank you so much 😃

[michaelperel]

@andoks As of the previous pull request,docker-lock now supports the ** pattern, which will recursively search. It relies on the z-glob package. Thank you for your suggestion of using a third party library, and I totally agree with you that this behavior should be language agnostic/people expect the double star pattern to work.

Hopefully, you will not have to use find/paste anymore!

As for the comma syntax for lists, I will add better documentation for all of the commands in the README, and re-evaluate the help text. Thanks!

[andoks]

@michaelperel Thank you. Now I have two options that will probably both work for my use-case: the CSV and globs.

I tested latest master, and it seems to work flawlessly 😄

go mod init test
go get github.com/safe-waters/docker-lock/cmd/docker-lock@dcd3893fb9cdcfc06667797f38c2eda798aa77ae
go run github.com/safe-waters/docker-lock/cmd/docker-lock lock generate --dockerfile-globs '**/*dockerfile' --composefile-globs '**/*docker-compose.yml' --dockerfile-recursive --composefile-recursive

I passed the recursive options to make sure that the dockerfiles called the default 'Dockerfile' name also is picked up (and compose-files too, although that should not be necessary for my repo) since the globs (as expected) are case sensitive.