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

Ignore dev and umbrella dependencies. #6

Merged
merged 4 commits into from
May 25, 2021

Conversation

gergo-papp
Copy link
Contributor

@gergo-papp gergo-papp commented Jun 15, 2020

Overview

I'm using kudos to generate a notices file that contains all the copyright and license requirements when distributing software to clients. We normally have several applications in the same mix project (having one top-level applications and other umbrella dependencies). We also have some local-only or dev dependencies that we use for testing (kudos is also one of these).

Problem

  • kudos doesn't work well with umbrella projects, because it cannot figure out most of the dependency informations (e.g. there is no checksum or license type for these projects)
  • since usually only production dependencies are relevant it should be possible to filter out dev-only dependencies

Implementation

  • Only run on top-level app (set @recursive to false). This means that we are only examining the top-level mix file. This is enough every dependency is already listed here and at least we don't end up with duplicates
  • Ignore umbrella dependencies or dependencies that are dependencies of umbrella deps
  • Ignore dev-only dependencies

Future dev

Ideally we would add --dev, --umbrella or similar CLI options

Only run on top-level app (set @recursice to false)
@dschniepp dschniepp assigned dschniepp and gergo-papp and unassigned dschniepp Jun 16, 2020
@dschniepp dschniepp self-requested a review June 16, 2020 18:58
@dschniepp
Copy link
Owner

Thanks for the PR. The points you mention make sense and I would be open to put it in the lib.

One point regarding @recursive, if I remember correctly we needed this for checking the dependencies of the dependencies. But if I understand you correctly these are now already part of the top-level?

@gergo-papp
Copy link
Contributor Author

gergo-papp commented Jun 17, 2020

Thanks for the PR. The points you mention make sense and I would be open to put it in the lib.

One point regarding @recursive, if I remember correctly we needed this for checking the dependencies of the dependencies. But if I understand you correctly these are now already part of the top-level?

Yes, that’s correct. At least that’s what I’ve seen locally. We definitely need a test case to validate this but it should work

I’m already using these changes in my forked repo - I’ll try to upstream once I get it into a clean state

@dschniepp
Copy link
Owner

@gergo-papp any updates on this PR?

@gergo-papp gergo-papp changed the title [Proposal] Ignore dev and umbrella dependencies. Ignore dev and umbrella dependencies. May 25, 2021
@gergo-papp gergo-papp force-pushed the ignore-dev-and-umbrella branch from c2e760e to 981f475 Compare May 25, 2021 17:18
@gergo-papp gergo-papp marked this pull request as ready for review May 25, 2021 17:21
@gergo-papp
Copy link
Contributor Author

@gergo-papp any updates on this PR?

Should be ready! I've been running this in "production" on several repos for the last year and it all seems fine. Not sure if we can add some actual tests...

Also: With this change running mix kudos.generate inside this repo will only show the report for decimal which is the only prod dependency - which makes sense, because you are not distributing any other dependencies

Copy link
Owner

@dschniepp dschniepp left a comment

Choose a reason for hiding this comment

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

Thanks for updating the PR

lib/kudos.ex Outdated Show resolved Hide resolved
@dschniepp dschniepp added this to the 0.3.0 milestone May 25, 2021
@gergo-papp gergo-papp force-pushed the ignore-dev-and-umbrella branch from 00b3f1c to 3c3a42e Compare May 25, 2021 18:08
@gergo-papp gergo-papp requested a review from dschniepp May 25, 2021 18:09
@dschniepp dschniepp merged commit cd08d41 into dschniepp:master May 25, 2021
@dschniepp
Copy link
Owner

Thank you very much :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants