Skip to content
This repository was archived by the owner on May 3, 2022. It is now read-only.

Refactoring: move docker driver Simulate field as a config parameter.#673

Open
silvin-lubecki wants to merge 1 commit intocnabio:masterfrom
silvin-lubecki:simulate-is-config
Open

Refactoring: move docker driver Simulate field as a config parameter.#673
silvin-lubecki wants to merge 1 commit intocnabio:masterfrom
silvin-lubecki:simulate-is-config

Conversation

@silvin-lubecki
Copy link
Contributor

Small refactoring, as I think this Simulate field has nothing to do here, since there's a config map for this kind of parameters.

Signed-off-by: Silvin Lubecki silvin.lubecki@docker.com

Copy link
Contributor

@michelleN michelleN left a comment

Choose a reason for hiding this comment

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

I believe you'll also want to add SIMULATE to this list of config options on the docker_driver here.

@silvin-lubecki
Copy link
Contributor Author

@michelleN I don't understand your comment, I already made this change, is it right?

@silvin-lubecki
Copy link
Contributor Author

PTAL @michelleN @radu-matei

Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

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

As the Simulate field is only present in the Docker driver, and there is already another way of passing config, I think we should be consistent -- and this approach is the most straightforward.

That being said, also have a look at the Docker driver test below:

$ make test
go test  ./...
# github.com/deislabs/duffle/pkg/driver [github.com/deislabs/duffle/pkg/driver.test]
pkg/driver/driver_test.go:58:19: d.(*DockerDriver).Simulate undefined (type *DockerDriver has no field or method Simulate)

Thanks!

@silvin-lubecki
Copy link
Contributor Author

Woops, thank you for spotting that @radu-matei ! But that's weird the CI didn't catch it 🤔
I'll fix that right away 👍

@radu-matei
Copy link
Member

Curious how none of the two CIs kicked in at all. Investigating..

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
@silvin-lubecki
Copy link
Contributor Author

Should be fixed + rebased 👍

@silvin-lubecki
Copy link
Contributor Author

PTAL @radu-matei @michelleN

1 similar comment
@silvin-lubecki
Copy link
Contributor Author

PTAL @radu-matei @michelleN

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants