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

Remove duplicated Shared module #11670

Merged
merged 13 commits into from
Feb 25, 2025
Merged

Conversation

robaiken
Copy link
Contributor

@robaiken robaiken commented Feb 24, 2025

What are you trying to accomplish?

  • Removing duplicated code between Docker and Docker Compose
  • Using original Docker classes in the Docker Compose implementation

This eliminates technical debt created during the beta release

Anything you want to highlight for special attention from reviewers?

Please review how Docker Compose now imports and extends the original Docker classes instead of using duplicated code.

How will you know you've accomplished your goal?

  • All tests pass
  • Docker Compose works as before
  • Future Docker changes only need to be implemented once

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@github-actions github-actions bot added the L: docker:compose Docker Compose label Feb 24, 2025
@robaiken robaiken self-assigned this Feb 24, 2025
@robaiken robaiken force-pushed the robaiken/remove-duplicated-shared-module branch from 08a64e7 to 3224c15 Compare February 24, 2025 15:03
@robaiken robaiken marked this pull request as ready for review February 24, 2025 15:03
@robaiken robaiken requested a review from a team as a code owner February 24, 2025 15:03
Copy link
Member

@randhircs randhircs left a comment

Choose a reason for hiding this comment

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

Files like version docker_compose/ version are not in use then we can remove that file and use docker/version file directly.

@robaiken
Copy link
Contributor Author

@randhircs Actually, we need to keep those files, they're what register the core classes that Dependabot needs to generate updates.

@robaiken robaiken requested a review from randhircs February 24, 2025 16:23
@randhircs
Copy link
Member

@robaiken I am good if you feel that just an import of other file is needed to have the file in stead of using the original one but I would prefer to use the original version class in place of docker_composer.

@robaiken robaiken marked this pull request as draft February 24, 2025 17:17
@robaiken robaiken marked this pull request as ready for review February 24, 2025 17:21
@robaiken
Copy link
Contributor Author

@randhircs I have removed the files

Dependabot::Utils.register_version_class("docker_compose", Dependabot::Docker::Version)
Dependabot::UpdateCheckers.register("docker_compose", Dependabot::Docker::UpdateChecker)
Dependabot::Utils.register_requirement_class("docker_compose", Dependabot::Docker::Requirement)
Dependabot::MetadataFinders.register("docker_compose", Dependabot::Docker::MetadataFinder)
Copy link
Contributor

@kbukum1 kbukum1 Feb 24, 2025

Choose a reason for hiding this comment

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

That's a better approach. We just need to ensure that Docker sources are imported when Docker Compose is imported. The package should explicitly require the Docker code.

@@ -23,3 +23,4 @@ USER dependabot
COPY --chown=dependabot:dependabot docker_compose $DEPENDABOT_HOME/docker_compose
COPY --chown=dependabot:dependabot common $DEPENDABOT_HOME/common
COPY --chown=dependabot:dependabot updater $DEPENDABOT_HOME/dependabot-updater
COPY --chown=dependabot:dependabot docker $DEPENDABOT_HOME/docker
Copy link
Contributor

Choose a reason for hiding this comment

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

That's good point since we need docker sources.

@kbukum1
Copy link
Contributor

kbukum1 commented Feb 24, 2025

@robaiken,

Everything looks good to me! Just as I mentioned, you might want to explicitly require dependabot-docker ecosystem as a library for omnibus. Maybe doing like following will work:

require "dependabot/docker"

in docker_compose.rb to ensure the dependabot-docker library is properly loaded.

@robaiken robaiken force-pushed the robaiken/remove-duplicated-shared-module branch from 93aeada to 05e4d8a Compare February 25, 2025 11:01
@markhallen markhallen dismissed randhircs’s stale review February 25, 2025 11:27

The files suggested for removal were removed.

@robaiken robaiken merged commit 4d964f2 into main Feb 25, 2025
288 of 365 checks passed
@robaiken robaiken deleted the robaiken/remove-duplicated-shared-module branch February 25, 2025 11:57
dmitris pushed a commit to dmitris/dependabot-core that referenced this pull request Feb 26, 2025
* Remove duplicated `Shared` module

* use docker classes rather than recreate them

* bump typed

* remove unneeded registry files

* remove duplicated test

* removing update checker from reg

* use docker updater base image

* edger load docker

* Add docker as a dep to the docker_compose dep

* updating updater lock file

* adding docker image details

* reverting docker_compose image

* removing docker image section
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: docker:compose Docker Compose
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants