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.

Sign up for GitHub

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

Jump to bottom

"init" option to copy in a custom docker-compose.override.yml file #285

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

"init" option to copy in a custom docker-compose.override.yml file #285

wants to merge 7 commits into from

Conversation

Philip-21
Copy link
Contributor

This is pr fixes for #184

@Philip-21
first commit
fe29231 
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@Philip-21
correct implementations
15aa781 
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@Philip-21
Copy link
Contributor Author

Philip-21 commented Jan 18, 2024 •
edited

@nguyer , @awrichar i have been able to tackle this issue so far with implementations i have made please i would like reviews,
and i am wondering why the e2e tests are failing . I tested everything out and they worked fine, below are the snapShot results

@Philip-21
Copy link
Contributor Author

Philip-21 commented Jan 18, 2024 •
edited

proof-oo1

i tested the command ff init --override "docker-compose.yml" to copy the contents from the docker-compose file to the docker-override file

@Philip-21
Copy link
Contributor Author

hyperledger-1

The docker-compose.yml file's content gets copied to the docker-compose-override.yml file in the stack i created ?

@Philip-21
Copy link
Contributor Author

proof-oo2

the docker-compose.yml file

@nguyer
Copy link
Contributor

nguyer commented Jan 23, 2024

The E2E tests are failing with this error:

ff -v --ansi never init ethereum --prometheus-enabled --database sqlite3 firefly_e2e 2 --token-providers erc1155 --manifest ../../manifest.json --sandbox-enabled=false --multiparty=true
initializing new FireFly stack...
Error: failed to copy file data to docker-compose.override.yml: open : no such file or directory

nguyer
nguyer requested changes Jan 23, 2024
View reviewed changes
internal/stacks/stack_manager.go Outdated
Comment on lines 190 to 192
if err := s.copyToDockerComposeOverride(options.CustomPath); err != nil {
return fmt.Errorf("failed to copy file data to docker-compose.override.yml: %s", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need a check around this to see if options.CustomPath has been set or not. Right now the tests seem to indicate that the CLI fails to initialize a stack if this flag is not set.

@Philip-21
making changes
4f0cc56 
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@Philip-21
making changes
7be4ff3 
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@Philip-21
Copy link
Contributor Author

Philip-21 commented Jan 23, 2024 •
edited

@nguyer I have corrected it, all checks pass successfully, and the docker -override works

Philip-21 and others added 2 commits January 24, 2024 22:53
@Philip-21
Merge branch 'main' into docker-override
209e501 
Signed-off-by: Philip Obiora <73377830+Philip-21@users.noreply.github.com>
@Philip-21
reslove conflicts
587258e 
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
nguyer
nguyer requested changes Feb 6, 2024
View reviewed changes
internal/stacks/stack_manager.go Outdated
@@ -188,6 +188,11 @@ func (s *StackManager) InitStack(options *types.InitOptions) (err error) {
if err := s.writeDockerComposeOverride(compose); err != nil {
return fmt.Errorf("failed to write docker-compose.override.yml: %s", err)
}
if options.CustomPath == "docker-compose.yml" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this check now enforces that you can only ever pass in a file named docker-compose.yml in the current directory. I would think we would just want to check to see if this is an empty string here (and then maybe validate the file exists).

internal/stacks/stack_manager.go
Comment on lines +428 to +429
dockerPath := filepath.Join(s.Stack.StackDir, dockerComposePath)
overrideComposeContent, err := os.ReadFile(dockerPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading this right, this means we will read the automatically generated docker-compose.override.yml, append it to a comment, and write it back out again to the same file.

From my understanding the goal here is to allow the user to pass in a yaml file from anywhere on their system, and we will use that as or merge it with the existing docker-compose.override.yml file. I'm not sure this function actually creates any new functionality as-is.

@Philip-21
adjustments
36259c3 
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@Philip-21
Copy link
Contributor Author

Hi @nguyer is this pr still open or i should close it ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nguyer nguyer nguyer requested changes

@awrichar awrichar Awaiting requested review from awrichar

@peterbroadhurst peterbroadhurst Awaiting requested review from peterbroadhurst

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Philip-21 @nguyer