operator-registry icon indicating copy to clipboard operation
operator-registry copied to clipboard

Remove unused Dockerfiles

Open timflannagan opened this issue 3 years ago • 8 comments

Signed-off-by: timflannagan [email protected]

Description of the change: Remove the opm-example.Dockerfile and index.Dockerfile that are unused.

Motivation for the change: Tech debt.

Reviewer Checklist

  • [ ] Implementation matches the proposed design, or proposal is updated to match implementation
  • [ ] Sufficient unit test coverage
  • [ ] Sufficient end-to-end test coverage
  • [ ] Docs updated or added to /docs
  • [ ] Commit messages sensible and descriptive

timflannagan avatar Jul 28 '22 17:07 timflannagan

Codecov Report

Merging #999 (c0e826c) into master (6ac352f) will not change coverage. The diff coverage is n/a.

:exclamation: Current head c0e826c differs from pull request most recent head 8c386e9. Consider uploading reports for the commit 8c386e9 to get more accurate results

@@           Coverage Diff           @@
##           master     #999   +/-   ##
=======================================
  Coverage   52.87%   52.87%           
=======================================
  Files         104      104           
  Lines        9468     9468           
=======================================
  Hits         5006     5006           
  Misses       3529     3529           
  Partials      933      933           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6ac352f...8c386e9. Read the comment docs.

codecov[bot] avatar Jul 28 '22 17:07 codecov[bot]

/approve

grokspawn avatar Aug 03 '22 19:08 grokspawn

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grokspawn, timflannagan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci[bot] avatar Aug 03 '22 19:08 openshift-ci[bot]

These two Dockerfiles are more or less examples that can be used for development. I did use then from time to time to build index images. It's not a bad thing to keep some example manifests around given this is an upstream repo.

dinhxuanvu avatar Aug 03 '22 19:08 dinhxuanvu

The problem is there's some many Dockerfiles in the repository, and having to potentially accommodate them while doing routine maintenance just increases that effort from my perspective. In terms of these specific Dockerfiles: if their role is simply to be consumed as examples, then I'm less inclined to keep them around.

timflannagan avatar Aug 03 '22 20:08 timflannagan

The problem is there's some many Dockerfiles in the repository, and having to potentially accommodate them while doing routine maintenance just increases that effort from my perspective. In terms of these specific Dockerfiles: if their role is simply to be consumed as examples, then I'm less inclined to keep them around.

If the rationale is to remove examples, then there are others to purge as well. If the concern is they are not organized, then I recommend to put them in a directory dedicated for development/examples or alike.

dinhxuanvu avatar Aug 03 '22 20:08 dinhxuanvu

I think my underlying motivation here is to start cutting around the edges of the registry repository, and remove some of the cruft to help work towards removing the vendor/ directory over time. That should help unlock upstream-downstream automation given the frequency of merge conflicts in that domain.

If we'd prefer to house the example Dockerfiles in a dedicated directory, then that's fine with me, but I'd still argue those dockerfile still require maintenance over time.

timflannagan avatar Aug 03 '22 21:08 timflannagan

It seems that we need to commit that either these dockerfiles are:

  1. useful reference and should be maintained
  2. with no special merit and an update burden

I'm of the opinion that the downstream one (index.Dockerfile) falls into the second case and doesn't need to be maintained in an upstream repo.
Given that the other one is "opm-example.Dockerfile" and doesn't have dependencies outside this repo, it sounds like it falls into the first case.

grokspawn avatar Aug 04 '22 12:08 grokspawn