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

feat(driver_test.go): add test around docker driver config #1012

Merged
merged 3 commits into from
Apr 29, 2020

Conversation

vdice
Copy link
Member

@vdice vdice commented Apr 22, 2020

What does this change

  • adds a test around Docker driver config based on runtime extension data

Depends on cnabio/cnab-go#206 or some variant thereof

TODO:

  • remove branch override in go.mod

What issue does it fix

Addresses a TODO lingering in code

Notes for the reviewer

Am I go mod-ing right?! All of the updated deps are coming from my branch override, right? Or is something up with my local Go cache/env that's bringing all of these in?

Checklist

  • Unit Tests
  • Documentation
  • Schema (porter.yaml)

github.com/spf13/viper v1.3.2
github.com/stretchr/testify v1.4.0
github.com/spf13/viper v1.4.0
github.com/stretchr/testify v1.5.1
Copy link
Member

Choose a reason for hiding this comment

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

cnab-go pulls in a higher version of testify and packr. The packr bump pulled in a new dependency (envy), etc... It was a cascade from bumping cnab-go and its dependencies.

@carolynvs
Copy link
Member

I don't see what caused you to need the changes in the PR to cnab-go? Can you help explain what in Porter made you need the change? I'm sure there was something that made this originally hard to work with.

@vdice
Copy link
Member Author

vdice commented Apr 23, 2020

I don't see what caused you to need the changes in the PR to cnab-go?

I added some notes in cnabio/cnab-go#206 (comment) but found we'd hit panics when applying configuration options to an otherwise uninitialized Driver instance (meaning, not actually running exec on the driver, where much of the initialization occurs), because the containerCfg and containerHostCfg members were also uninitialized (invalid references). I was hoping to support the ability to do the aforementioned, and so thought to change those fields to actual values.

@vdice vdice force-pushed the feat/add-docker-driver-test branch from 4c37176 to f03e974 Compare April 24, 2020 22:07

containerHostCfg, err := dockerish.GetContainerHostConfig()
require.NoError(t, err)
require.Equal(t, true, containerHostCfg.Privileged)
Copy link
Member

Choose a reason for hiding this comment

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

This is perfect. Thanks for your work getting the plumbing into cnab-go to support it.

Can you add a converse test to verify that when we don't have privileged that it's not set on the driver? Just to check for us hard coding it accidentally like it was originally.

Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

🚢

@carolynvs carolynvs merged commit e4aa6cd into getporter:master Apr 29, 2020
@vdice vdice deleted the feat/add-docker-driver-test branch April 30, 2020 17:05
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.

2 participants