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

first draft for supporting docker-compose v2 #232

Merged
merged 1 commit into from Jan 10, 2023

Conversation

ggessner
Copy link
Contributor

@ggessner ggessner commented Jan 6, 2023

Pull Request

Disclaimer

I'm not a golang developer, so any code presented here is a collection of educated guesses as how to solve the problem.
With that out of the way, let's present the pull request.

Problem

This pull request adresses Issue231: firefly fails check for docker compose v2. The problem arises due to docker changing the syntax for docker-compose from docker-compose in version 1 to docker compose in version 2. There is always the workaround to require the standalone docker-compose in version 1.

Fix

The fix is split into two parts, adressing the checking of the version and integrating the information into the stack manager that executes the docker commands via RunDockerComposeCommand. The information about which version syntax to use for the commands is carried via context.

CheckDockerConfig

the function that checks for the prerequisites when executing docker-compose commands, CheckDockerConfig returns a tuple (DockerComposeVersion, error) with DockerComposeVersion as enum to differentiate between None, ComposeV1 and ComposeV2. This value is stored in a context that is later transferred into the stack manager.
Unfortunately, cobra with version 1.4.0 does not provide the capability to allow context to be attached to a command.
Only from version 1.5.0, this feature has been merged, for reference: support for SetContext.
The solution using cobra 1.5.0 concerns all commands which use both PreRunE and RunE while also requiring the docker compose version to be transferred as context between these two executions:

        PreRunE: func(cmd *cobra.Command, args []string) error {
		ctx := ...
		version, err := docker.CheckDockerConfig()
		ctx = context.WithValue(ctx, docker.ComposeVersion{}, version)
		cmd.SetContext(ctx)
		return err
	},
	RunE: func(cmd *cobra.Command, args []string) error {
                ...
		stackManager := stacks.NewStackManager(cmd.Context())
               ...
	},

The functions currently affected are defined in:

  • accounts_create.go
  • accounts_list.go
  • deploy_ethereum.go
  • deploy_fabric

No version update is required when simply combining both executions, similar to functions from:

  • info.go
  • logs.go
  • reset.go
  • start.go
        RunE: func(cmd *cobra.Command, args []string) error {
		ctx := ...
		version, err := docker.CheckDockerConfig()
		ctx = context.WithValue(ctx, docker.ComposeVersion{}, version)
                if err != nil {
                     return err
                }
		stackManager := stacks.NewStackManager(cmd.Context())
               ...
	},

RunDockerComposeCommand

The second part of the changes affects the way the stack manager runs docker-compose functions. By retrieving the docker-compose version, either docker-compose or docker compose can be executed

Possible Tweaks

  • Instead of storing a version in the context, the command itself can be stored; when docker-compose in version 2 is detected, instead storing V1, the string docker compose is attached to the context with ComposeCmd as key. Consequently, the RunDockerComposeCommand function simply executes exec.Command(ctx.Value(ComposeCmd{}), command...).

Open Question

  • Is the separation between PreRunE and RunE necessary, because currently only docker checks are executed in PreRun. Small commands do not even specify PreRunE at all.

@nguyer
Copy link
Contributor

nguyer commented Jan 9, 2023

Hey @ggessner! Thank you so much for contributing this! I will review it and leave some feedback. I definitely looked into supporting Docker Compose V2 quite a while ago, but if I recall correctly there was some fundamental feature that Docker itself no longer supported, but we relied on heavily. Let me look into this and get back to you.

@nguyer
Copy link
Contributor

nguyer commented Jan 9, 2023

Okay, actually you can disregard my previous concern. That was an issue with the Compose Specification version (and going to V3), not the Docker Compose binary version.

Copy link
Contributor

@nguyer nguyer left a comment

Choose a reason for hiding this comment

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

This is a really great contribution, especially for a first contribution to the project! Thank you so much!

I think a couple of minor changes are needed, but after that, I would love to merge this and include it in the next release.

internal/docker/docker_checks.go Outdated Show resolved Hide resolved
internal/docker/docker.go Outdated Show resolved Hide resolved
internal/docker/docker.go Outdated Show resolved Hide resolved
@nguyer
Copy link
Contributor

nguyer commented Jan 10, 2023

This looks great now! The one last thing is to make sure you have sign-off on your commits. There are some details on how to do that here: https://github.com/hyperledger/firefly-cli/pull/232/checks?check_run_id=10537046269 It may be easiest to squash merge with one new commit with sign off. Sorry for the trouble, but this is just a requirement for all Linux Foundation repos.

@ggessner
Copy link
Contributor Author

Hi @nguyer,

thanks for the advice. I squashed the commits and signed the resulting one.
Let me know if I made any errors.

@nguyer
Copy link
Contributor

nguyer commented Jan 10, 2023

Sorry, it's not a signed commit that we need, but rather sign off. You just need to add -s to your commit message and git will do it for you:

https://git-scm.com/docs/git-commit#Documentation/git-commit.txt--s

Here's a bit more info about it if you need it: https://wiki.linuxfoundation.org/dco

Signed-off-by: Gernot Gessner <gegessner@outlook.de>
@ggessner
Copy link
Contributor Author

That felt like an unnecessary mistake from my side.
It should be fixed now.

@nguyer
Copy link
Contributor

nguyer commented Jan 10, 2023

Looks great now! Thank you for contributing this!

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.

None yet

2 participants