Skip to content
This repository has been archived by the owner on Dec 9, 2022. It is now read-only.

Use dep to manage dependencies #90

Closed
wants to merge 1 commit into from
Closed

Use dep to manage dependencies #90

wants to merge 1 commit into from

Conversation

jhernand
Copy link

Currently the image inspector uses the trash tool to manage the
dependencies, using the vendor.conf file to declare them, and
commiting vendor directory to the source code repository.

This patch changes the project to use the dep tool and removes the
vendor directory from the source code repository. The dependencies
will be downloaded by the dep tool when running make.

Currently the image inspector uses the `trash` tool to manage the
dependencies, using the `vendor.conf` file to declare them, and
commiting `vendor` directory to the source code repository.

This patch changes the project to use the `dep` tool and removes the
`vendor` directory from the source code repository. The dependencies
will be downloaded by the `dep` tool when running `make`.

Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
@enoodle
Copy link

enoodle commented Jan 17, 2018

@jhernand what are the benefits of commiting the file Gopkg.lock ?
Seems like it is changed by dep ensure when the unconstrained dependencies are updated. It is similar to me to a Gemfile.lock.

@jhernand
Copy link
Author

The advantage of having the Gopkg.lock file in the repository is that if you use dep ensure -vendor-only then it will get exactly the versions dependencies that you have listed in that file. Note that in the Makefile we are using that -vendor-only option, so the unconstrained dependencies will not be updated. Only if you run dep ensure manually will they be updated.

Copy link

@enoodle enoodle left a comment

Choose a reason for hiding this comment

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

I am very much for this,
When we moved to trash there was no clear standard but dep seems to be it now.
Also, not including vendor directory is a nice move in my opinion. It will be lighter repository and clearer for PRs that are adding/changing dependencies.

@simon3z
Copy link

simon3z commented Jan 19, 2018

@jhernand have you checked #68? At that time dep was not able to properly identify the dependencies resulting in bloated libraries inclusion. Have you checked that when moving to #85 we won't have dependency issues? (e.g. what's the result with trash vs dep?)

@jhernand
Copy link
Author

jhernand commented Jan 25, 2018

My understanding from #68 and from how dep works is that it downloads more dependencies because it doesn't ignore tests. That means that after this patch there will be still much more files in the the vendor directory when using dep than when using trash. But note that the patch also changes the project so that the vendor directory will not be committed into the git repository, so it drastically reduces the set of files of the project (not of the repository itself, as old vendor contents will still be in the history).

As for #85, I think it will actually improve it a lot; the commit will go from 912 changed files to less than 20, which is good for review, for example.

@enoodle
Copy link

enoodle commented Jan 25, 2018

We can also change the way that we build the container image to not copy the whole project and build it on the image, but create the binary file locally and only copy that.
It will create smaller images as they won't need the golang compilers installed and will ignore the added files that dep is coping. This is the way that Prometheus is building the container image.

@jhernand
Copy link
Author

Indeed, actually I wanted to do exactly that, and this was the first step.

@enoodle
Copy link

enoodle commented Jan 25, 2018

@jhernand I think I learned of this option from you and from @zgalor

@zgalor
Copy link

zgalor commented Jan 25, 2018

👍 for dep.
Regarding building the binary outside of the Dockerfile, This will probably break docker hub auto-build

@jhernand
Copy link
Author

To overcome the problem with the docker hub auto-build we could maybe modify the Travis configuration so that it builds and pushes the image to docker hub.

@simon3z
Copy link

simon3z commented Jan 26, 2018

As for #85, I think it will actually improve it a lot; the commit will go from 912 changed files to less than 20, which is good for review, for example.

Nice! 👍
@jhernand do you want to queue this after #91 ?

@enoodle
Copy link

enoodle commented Feb 8, 2018

ping @jhernand Can you rebase this on the changes from #91 please?

@openshift-bot
Copy link

@jhernand: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2018
@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 17, 2020
@openshift-bot
Copy link

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci-robot openshift-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 16, 2020
@openshift-bot
Copy link

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

@openshift-ci-robot
Copy link

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants