Skip to content

vendor: remove vendor ;) - #5856

Closed
stevvooe wants to merge 4 commits into
docker:masterfrom
stevvooe:sjd/remove-vendoring
Closed

vendor: remove vendor ;)#5856
stevvooe wants to merge 4 commits into
docker:masterfrom
stevvooe:sjd/remove-vendoring

Conversation

@stevvooe

@stevvooestevvooe commented Feb 21, 2025

Copy link
Copy Markdown
Contributor

The current vendoring setup makes it difficult for new developers to get started on the repository. It seems to be this way to address the problems with adoption across the Docker and Moby repositories. At this point, many of those issues seem to be behind us so it's worth a try again to make this project simpler to work on.

- What I did

Removed vendoring. I did it with a hammer, so we might need to make some repairs.

- How I did it

With a hammer. 🔨

- How to verify it

We should see CI build as normal. Downstream tests, such as moby, should pass as normal.

There were some areas that were using the term "vendor" in a plugin context. We should see NO regressions in that behavior.

- A picture of a cute animal (not mandatory but encouraged)

image

@stevvooe
stevvooe requested review from a team and thaJeztah as code ownersFebruary 21, 2025 18:13
@stevvooe

Copy link
Copy Markdown
ContributorAuthor

We might want to include #4228 as part of this.

@codecov-commenter

codecov-commenter commented Feb 21, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.27%. Comparing base (77a8a8c) to head (4fd1866).

Additional details and impacted files
@@ Coverage Diff @@## master #5856 +/- ##
==========================================
- Coverage 59.30% 59.27% -0.03% 
==========================================
Files 353 353 Lines 29694 29694 ==========================================
- Hits 17609 17601 -8 - Misses 11104 11113 +9 + Partials 981 980 -1 

@thaJeztahthaJeztah 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.

not LGTM - this breaks the release pipeline, among others.

@stevvooe

Copy link
Copy Markdown
ContributorAuthor

this breaks the release pipeline, among others.

Ok, how do we do this so it doesn't break the release pipeline?

The current vendoring setup makes it difficult for new developers to get
started on the repository. It seems to be this way to address the
problems with adoption across the Docker and Moby repositories. At this
point, many of those issues seem to be behind us so it's worth a try
again to make this project simpler to work on.
Signed-off-by: Stephen Day <stephen.day@docker.com>
Signed-off-by: Stephen Day <stephen.day@docker.com>
Signed-off-by: Stephen Day <stephen.day@docker.com>
Signed-off-by: Stephen Day <stephen.day@docker.com>
@laurazard

Copy link
Copy Markdown
Member

I assume that by "remove vendoring" what this does is "migrate to using go modules" – having a go.mod at the root of the repo means that we're opting into a whole new contract, and potentially impacts how future development can be done, as well as all downstream consumers of the CLI, so it should definitely not be done lightly.

@stevvooe

Copy link
Copy Markdown
ContributorAuthor

@laurazard I think the primary issue here is the current setup makes it harder to contribute to cli and I thought I could solve that. Unfortunately, I underestimated how broken v2+ Go modules actually are and it seems like this will be a huge amount of work to get there. I was unable to mock out a solution to get the tags right in my fork and make this actually work.

Docker cli is an easy build, so this is a bit disappointing. I'll close this out and let the maintainers figure it out.

Sign up for freeto 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

@stevvooe@codecov-commenter@laurazard@thaJeztah