-
Notifications
You must be signed in to change notification settings - Fork 54
Refactoring: move docker driver Simulate field as a config parameter. #673
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
@michelleN I don't understand your comment, I already made this change, is it right? |
PTAL @michelleN @radu-matei |
There was a problem hiding this 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!
Woops, thank you for spotting that @radu-matei ! But that's weird the CI didn't catch it 🤔 |
Curious how none of the two CIs kicked in at all. Investigating.. |
Signed-off-by: Silvin Lubecki <[email protected]>
c7f3a46
to
0bb798e
Compare
Should be fixed + rebased 👍 |
PTAL @radu-matei @michelleN |
1 similar comment
PTAL @radu-matei @michelleN |
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 [email protected]