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 all extendable code into ext/ #305

Merged
merged 23 commits into from
Feb 6, 2017
Merged

Refactor all extendable code into ext/ #305

merged 23 commits into from
Feb 6, 2017

Conversation

jzelinskie
Copy link
Contributor

@jzelinskie jzelinskie commented Jan 14, 2017

This is a WIP PR to move everything into one flat directory of extensions. This should make it easier for programmers to discover everything customizable with Clair. It also vastly simplifies the structure of the codebase.

My goals are as follows:

  • refactor fetchers into ext/vulnsrc
  • refactor metadata fetchers into ext/vulnmdsrc
  • refactor data detectors into ext/imagefmt
  • refactor namespace detectors into ext/featurens
  • refactor feature detectors into ext/featurefmt
  • lock all extension lists with sync.RWMutex
  • update all documentation to refer to new names and project structure
  • convert utils/ into idiomatic pkg/
  • flatten notifier, worker, updater, config packages into top-level clair package

I'm completely open to discussion around naming (naming is hard) and further refactors to simplifying the structure of the project. This is a big change, so navigating the branch directly might be easier to understand at a high-level than the GitHub diff.

@jzelinskie jzelinskie added area/dev tooling related to tools used by developers component/fetcher kind/cleanup no functional changes, only reorganization kind/design relates to the fundamental design of a component reviewed/needs rework will be closed if review not addressed labels Jan 14, 2017
@jzelinskie jzelinskie added this to the 2.0 milestone Jan 14, 2017
@Djelibeybi
Copy link
Contributor

My thoughts on naming:

ext/vulnsrc => ext/erratasrc (because it actually pulls the entire errata list including security, bugfix and enhancements)
ext/vuldmsrc => ext/cvesrc (because it's pulling CVE-related metadata)
ext/featurefmt => ext/packagefmt (because its package format based)

Though, I'm not going to die on a hill for any of them. :)

@jzelinskie
Copy link
Contributor Author

ext/vulnsrc => ext/erratasrc (because it actually pulls the entire errata list including security, bugfix and enhancements)

Is errata common vocab in this space? I haven't noticed it.

ext/vuldmsrc => ext/cvesrc (because it's pulling CVE-related metadata)

I think the vocab among the project is vulnerability because we don't want to limit the scope to just "CVEs".

ext/featurefmt => ext/packagefmt (because its package format based)

We've been calling these features because you could hypothetically use Clair to detect vulnerabilities via file hashes and not necessarily just packages.

@Djelibeybi
Copy link
Contributor

We use errata as the term for updates to the OS, whether security, bugfix or enhancement. The same term is used by our upstream vendor and their other downstream distributions. It may not be commonly used by Debian (-based) distros, though.

This puts config in its relevant location and moves functions around
loading config files into the main package.

As a side effect of removing cyclic imports for the API config, the
context library is no longer used.
@Quentin-M
Copy link
Contributor

Quentin-M commented Jan 27, 2017

This PR is more than welcome appreciated.

Few comments:

  • Even though this PR is meant to increase readability and discoverability, I find it hard to understand which ext folder contains what. If you are to consider that fmt refers to parsers, then featurens should have it too. Maybe namespacefmt? Shouldn't notification be notifier? vulnmdsrc is an hard one..
  • We could have a simply README.md in the ext/ folder, describing the role of each subfolder.
  • Why isn't the database considered an extension too?
  • Is there anything in the foreseeable future that would affect the way we do this refactoring? For instance, we might think about clairctl or a Kubernetes integration. Where would that land? Ideally, they'd be small enough to fit into cmd/, while leveraging some functions from pkg/ (e.g. rkt/docker pulling).

I am not strongly against the word errata and I understand it is used by many vendors. However I'd like to keep our vocabulary consistent across the project / code. If we start calling a package errata, then the structures (and API) should change too in my opinion, which is too big of a change to be justified. We've been trying to avoid the term CVE for the reason explained above.

@jzelinskie
Copy link
Contributor Author

jzelinskie commented Jan 27, 2017

Even though this PR is meant to increase readability and discoverability, I find it hard to understand which ext folder contains what. If you are to consider that fmt refers to parsers, then featurens should have it too. Maybe namespacefmt? Shouldn't notification be notifier? vulnmdsrc is an hard one..

I'm totally open to better suggestions for names. You have to consider both the name of the package and the interface, though. notification makes more sense when you see that when called it's notification.Sender.

We could have a simply README.md in the ext/ folder, describing the role of each subfolder.

I've tried this in a few other projects and found it counter-intuitively hurts discoverability of documentation. This is my anecdote. We should also strive to follow CoreOS OSS standards with our Documentation directory.

Why isn't the database considered an extension too?

I absolutely think it should be, but right now there are definitely implicit dependencies on Postgres implementation. I don't feel confident telling people they really could swap out the database at this point.

Is there anything in the foreseeable future that would affect the way we do this refactoring? For instance, we might think about clairctl or a Kubernetes integration. Where would that land? Ideally, they'd be small enough to fit into cmd/, while leveraging some functions from pkg/ (e.g. rkt/docker pulling).

I think that clairctl should exist in cmd/ and a Kubernetes integration should exist outside of this repository. I honestly like to delete contrib as soon as possible. People shouldn't be becoming dependent on analyze-local-images and out of date Kubernetes resources...

@jzelinskie jzelinskie merged commit eb5be92 into quay:master Feb 6, 2017
@jzelinskie jzelinskie deleted the ext branch February 6, 2017 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dev tooling related to tools used by developers kind/cleanup no functional changes, only reorganization kind/design relates to the fundamental design of a component reviewed/needs rework will be closed if review not addressed
Development

Successfully merging this pull request may close these issues.

3 participants