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

Refactor image preprocessing #121

Merged
merged 4 commits into from
Dec 8, 2018
Merged

Conversation

wagoodman
Copy link
Owner

This was done for two reasons:

  • preprocessing can only handle docker images. It would be ideal to one day expand this to other image formats
  • this was already explicitly labeled as tech debt

@wagoodman wagoodman added the wip label Dec 2, 2018
@wagoodman wagoodman added this to the v0.5.0 milestone Dec 2, 2018
@wagoodman wagoodman force-pushed the refactor-image-preprocessing branch from 7018cb3 to 93fa41b Compare December 2, 2018 03:00
@wagoodman wagoodman self-assigned this Dec 2, 2018
@ccinelli
Copy link

ccinelli commented Dec 3, 2018

It looks like this just re-shuffle code in different files. Am I wrong?

@wagoodman
Copy link
Owner Author

wagoodman commented Dec 3, 2018

For now, yes... this is still a work in progress. This abstracts the steps for any format that could be added later (such as OCI). I still need to refactor the Parse(), Analyze(), and read() sections a bit more. Another benefit is that this abstraction does not bail via an explicit exit, but instead by returning an error, which makes this much more testible (still need to add tests).

@willmurphyscode
Copy link
Collaborator

This code looks good, but I wish I knew more about how other image formats work. I'm worried that if we extract interfaces before we implement a second image analyzer, we'll make extra work for ourselves.

Would it be worth spiking an analyzer for runC images before we do this, and then seeing if all the interfaces we have still hold up?

Or maybe the formats are all similar enough that my worry is unfounded. @wagoodman do you feel confident enough in how the analysis of other images will be similar or different from Docker images to go ahead with this?

@wagoodman
Copy link
Owner Author

Your worry is a fair one, I'm not entirely certain this is the right interface (for instance, will we only need a string to fetch and parse an image? or something more complex to indicate what to fetch?). However, I do feel like it's in the right direction and it is as simple as it can be for now. Also, since we're not at 1.0.0 yet there isn't any obligation to ensure interfaces won't change or break against previous versions.

@wagoodman wagoodman force-pushed the refactor-image-preprocessing branch from d5ab4e0 to 927dd46 Compare December 8, 2018 16:37
@wagoodman wagoodman force-pushed the refactor-image-preprocessing branch from 927dd46 to 91a5108 Compare December 8, 2018 16:38
@wagoodman wagoodman merged commit 9f9a8f2 into master Dec 8, 2018
@wagoodman wagoodman deleted the refactor-image-preprocessing branch December 8, 2018 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants