Skip to content

fix: dockerignore - #104

Merged
gytelio merged 1 commit into
masterfrom
fix/build-local-dockerignore
Jul 16, 2026
Merged

fix: dockerignore#104
gytelio merged 1 commit into
masterfrom
fix/build-local-dockerignore

Conversation

@gytelio

@gytelio gytelio commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

We need to ignore dockerignore files same way we ignore gitignore. We need to ignore directories also like /tests, because otherwise we are getting "sourceFilesTooLarge\ error.
Found in @apify/consts :
MAX_MULTIFILE_BYTES = 3 * 1024 ** 2 // 3 MiB (3,145,728 bytes)

@gytelio
gytelio requested a review from metalwarrior665 July 15, 2026 08:10

@metalwarrior665 metalwarrior665 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We are already implementing dockerignore handling in #96

These are for different features so that would be fine but we would have duplicate utils for dockerignore. Can you check how it is handled in that PR and if you could reuse it or if it does overlap?

@gytelio

gytelio commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

We are already implementing dockerignore handling in #96

These are for different features so that would be fine but we would have duplicate utils for dockerignore. Can you check how it is handled in that PR and if you could reuse it or if it does overlap?

@metalwarrior665 I checked. The util function from #96 I cannot just plug it, the function and this one expect different kinds of input. But I would like to reuse it, I like the usage of the ignore package. But there is a bug that I was trying to fix with this one (I added a comment) https://github.com/apify/apify-test-tools/pull/96/changes#r3594171691.

I would like to do a cleanup after the 96 will be merged and the bug is fixed. But currently I would like to merge this one because it works and its tested, and some people currently wants to use the local build now.

@ruocco-l

Copy link
Copy Markdown
Contributor

@gytelio thank you for your comment on my PR. I think, as @metalwarrior665 already pointed out in my PR, this is a feature that astray from my original intent. I think we should go ahead and implement this PR first and then it would be my work to make it work in my PR, instead of doing it the other way around.

@metalwarrior665

Copy link
Copy Markdown
Member

Let's give that PR few days to get merged. Merging this one first would complicate it. But if it gets stuck then let's do it.

@metalwarrior665

Copy link
Copy Markdown
Member

Ok, so if @ruocco-l is ok rebasing on top of this one, we can go with it

@gytelio

gytelio commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

@metalwarrior665 Im not familiar with this. If I merge it to main will it automatically be on the latest release? Or do I need to run something manually?

@metalwarrior665

Copy link
Copy Markdown
Member

It will release a beta version. For regular version, you need Actions -> Stable release (patch version). I can do it

@gytelio
gytelio merged commit c041024 into master Jul 16, 2026
10 checks passed
@metalwarrior665

Copy link
Copy Markdown
Member

Released as 0.8.6

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.

4 participants