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

replace dockerfile/dockerignore with patternmatcher/ignorefile #4166

Merged
merged 3 commits into from
Aug 25, 2023

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Aug 22, 2023

relates to:


vendor: github.com/moby/patternmatcher v0.6.0

  • integrate frontend/dockerfile/dockerignore from buildkit

full diff: moby/patternmatcher@v0.5.0...v0.6.0

replace dockerfile/dockerignore with patternmatcher/ignorefile

The BuildKit dockerignore package was integrated in the patternmatcher
repository / module. This patch updates our uses of the BuildKit package
with its new location.

deprecate frontend/dockerfile/dockerignore

This package was moved to github.com/moby/patternmatcher/ignorefile.
Mark the alias as deprecated.

☝️ doing the deprecation in a separate commit, so that we can consider backporting the first commit to the release branch (without having to silence linters)

- integrate frontend/dockerfile/dockerignore from buildkit

full diff: moby/patternmatcher@v0.5.0...v0.6.0

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the migrate_dockerignore branch from b30edad to e3a26f3 Compare August 22, 2023 22:13
The BuildKit dockerignore package was integrated in the patternmatcher
repository / module. This patch updates our uses of the BuildKit package
with its new location.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This package was moved to github.com/moby/patternmatcher/ignorefile.
Mark the alias as deprecated.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the migrate_dockerignore branch from e3a26f3 to 376e9c8 Compare August 22, 2023 22:14
@thaJeztah thaJeztah changed the title migrate dockerfile/dockerignore to github.com/moby/patternmatcher/ignorefile replace dockerfile/dockerignore with patternmatcher/ignorefile Aug 22, 2023
@thaJeztah thaJeztah marked this pull request as ready for review August 22, 2023 22:16

// ReadAll is an alias for [ignorefile.ReadAll].
//
// Deprecated: use [ignorefile.ReadAll] instead.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this deprecation file

Copy link
Member Author

Choose a reason for hiding this comment

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

Without it, external consumers won't discover that it was moved, and will be caught off-guard when the alias is removed, and now need to search "where the heck" it went.

@thaJeztah
Copy link
Member Author

w.r.t. the deprecation message, I asked Tonis on slack if he could provide more details; let me post here;

we don’t have stability guarantees in the Go API. People will need to update their code anyway. Just extra maintenance work and inconsistencies.
buildkit should be a project where we iterate as fast as possible. If needed we should do more releases even if they are tiny instead of backports

My motivation for adding the deprecation comment is two-fold; I'd love to have these changes in with the potential to have them included in a v0.12.x patch release; I want to cut-down circular dependencies as much as possible (and as early as possible), as they currently are a blocker for some other repositories to complete the migration to Go modules.

However, moving this code means a breaking change for consumers of this code (which we "don't know who they are"). The deprecation comment should trigger linters, which helps them to migrate their code before it's actually removed, and to save them being caught off-guard when it is removed (having to try to figure out "where the h*ck that code went").

I'm not a maintainer though, so let me know if that's still a show-stopper, and if a breaking change is preferred (i.e., remove the alias) but also if that would make this patch "not qualified" for back-porting (#4170).

@tonistiigi tonistiigi merged commit c3c6578 into moby:master Aug 25, 2023
@thaJeztah thaJeztah deleted the migrate_dockerignore branch August 25, 2023 19:27
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