Skip to content
This repository has been 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
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

None yet

3 participants