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

Adds support for elixir sub-projects without the need for umbrella applications #3944

Merged
merged 2 commits into from
Jun 23, 2021
Merged

Conversation

gjsduarte
Copy link
Contributor

@gjsduarte gjsduarte commented Jun 20, 2021

This PR will change the hex file fetcher to include path dependencies from a monorepo main mix.exs file.
With this change, dependabot will now correctly update dependencies used in sub-projects, as long as they exist as path dependencies of your project.

Addresses #820

@gjsduarte gjsduarte requested a review from a team as a code owner June 20, 2021 16:53
@gjsduarte
Copy link
Contributor Author

gjsduarte commented Jun 20, 2021

Here is the content of the encoded mix.exs:

defmodule BankPlatform.Mixfile do
  use Mix.Project

  def project do
    [build_embedded: Mix.env == :prod,
     start_permanent: Mix.env == :prod,
     version: "1.0.0-dev",
     source_url: "https://github.com/wojtekmach/acme_bank",
     name: "Acme Bank",
     docs: [source_ref: "HEAD", main: "main", assets: "docs", extras: ["docs/main.md"]],
     deps: deps(),
     aliases: aliases()]
  end

  defp deps do
    [{:ex_doc, github: "elixir-lang/ex_doc", branch: "master", only: :dev},
     {:bank, path: "apps/bank", from_umbrella: true}]
  end

  defp aliases do
    ["ecto.setup": ["ecto.create", "ecto.migrate", "ecto.seed"],
     "ecto.seed": ["run ./apps/bank/priv/repo/seeds.exs"],
     "ecto.reset": ["ecto.drop", "ecto.setup"],
     "test": ["ecto.create --quiet", "ecto.migrate", "test"]]
  end
end

@jurre
Copy link
Member

jurre commented Jun 21, 2021

Thanks @gjsduarte, this updates the FileFetcher to ensure we pull in the right files, but it's not clear to me if the rest of the update process actually works with this setup.

It'd be awesome if you could add a few tests for the UpdateChecker and FileUpdater classes to demonstrate this and to make sure that we don't regress there in the future. LMK if I can help!

So far looks great and thanks for tackling this 🙇

@gjsduarte
Copy link
Contributor Author

Thanks for the feedback @jurre. I've added tests for the UpdateChecker and FileUpdater classes.

Basically, they ensure the rest of the process behaves the same as with umbrella applications.

Let me know what you think.

map { |f| File.join(apps_path, f.name) }
repo_contents(dir: apps_path).
select { |f| f.type == "dir" }.
map { |f| File.join(apps_path, f.name) } if apps_path
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there's already a guard against apps_path being nil on line 51

Suggested change
map { |f| File.join(apps_path, f.name) } if apps_path
map { |f| File.join(apps_path, f.name) }

@@ -0,0 +1,4 @@
%{"distillery": {:hex, :distillery, "1.5.2", "eec18b2d37b55b0bcb670cf2bcf64228ed38ce8b046bb30a9b636a6f5a4c0080", [:mix], [], "hexpm"},
Copy link
Contributor

Choose a reason for hiding this comment

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

We've got a new pattern for fixtures that would be neat to use here. Could you move these these files and the mixfile + lockfile into a project folder instead?

e.g. hex/spec/fixtures/projects/umbrella_sub_projects, so this would include all files needed for the update: apps/dependabot_business/mix.exs, apps/dependabot_web/mix.exs, mix.exs and mix.lock (you could copy these files from mixfiles/umbrella and lockfiles/umbrella).

Example layout for bundler: https://github.com/dependabot/dependabot-core/tree/11695abbfbd2c8c54db885c56a26a3f2fdc5bedb/bundler/spec/fixtures/projects/bundler1/custom_tag_gemfile

You can then replace let(:files) { [mixfile, lockfile, sub_mixfile1, sub_mixfile2] } with let(:files) { project_dependency_files("umbrella_sub_projects") }

Comment on lines 537 to 551
let(:mixfile_body) { fixture("mixfiles", "sub_projects") }
let(:lockfile_body) { fixture("lockfiles", "sub_projects") }
let(:files) { [mixfile, lockfile, sub_mixfile1, sub_mixfile2] }
let(:sub_mixfile1) do
Dependabot::DependencyFile.new(
name: "apps/dependabot_business/mix.exs",
content: fixture("mixfiles", "dependabot_business")
)
end
let(:sub_mixfile2) do
Dependabot::DependencyFile.new(
name: "apps/dependabot_web/mix.exs",
content: fixture("mixfiles", "dependabot_web")
)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Once you've got the project fixture set up this should work:

Suggested change
let(:mixfile_body) { fixture("mixfiles", "sub_projects") }
let(:lockfile_body) { fixture("lockfiles", "sub_projects") }
let(:files) { [mixfile, lockfile, sub_mixfile1, sub_mixfile2] }
let(:sub_mixfile1) do
Dependabot::DependencyFile.new(
name: "apps/dependabot_business/mix.exs",
content: fixture("mixfiles", "dependabot_business")
)
end
let(:sub_mixfile2) do
Dependabot::DependencyFile.new(
name: "apps/dependabot_web/mix.exs",
content: fixture("mixfiles", "dependabot_web")
)
end
let(:files) { project_dependency_files("umbrella_sub_projects") }

Comment on lines 182 to 197
let(:mixfile_fixture_name) { "sub_projects" }
let(:lockfile_fixture_name) { "sub_projects" }

let(:files) { [mixfile, lockfile, sub_mixfile1, sub_mixfile2] }
let(:sub_mixfile1) do
Dependabot::DependencyFile.new(
name: "apps/dependabot_business/mix.exs",
content: fixture("mixfiles", "dependabot_business")
)
end
let(:sub_mixfile2) do
Dependabot::DependencyFile.new(
name: "apps/dependabot_web/mix.exs",
content: fixture("mixfiles", "dependabot_web")
)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, once the project is set up:

Suggested change
let(:mixfile_fixture_name) { "sub_projects" }
let(:lockfile_fixture_name) { "sub_projects" }
let(:files) { [mixfile, lockfile, sub_mixfile1, sub_mixfile2] }
let(:sub_mixfile1) do
Dependabot::DependencyFile.new(
name: "apps/dependabot_business/mix.exs",
content: fixture("mixfiles", "dependabot_business")
)
end
let(:sub_mixfile2) do
Dependabot::DependencyFile.new(
name: "apps/dependabot_web/mix.exs",
content: fixture("mixfiles", "dependabot_web")
)
end
let(:files) { project_dependency_files("umbrella_sub_projects") }

Copy link
Contributor

@feelepxyz feelepxyz 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 the contribution 🙇 left a few minor comments otherwise LGTM!

@gjsduarte
Copy link
Contributor Author

Thanks for the feedback @feelepxyz! I've refactored the tests to use the new pattern as you suggested.
Let me know what you think

Copy link
Contributor

@feelepxyz feelepxyz 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 the spec changes 👌 I forgot to approve the action run so just waiting for ci to come back

Copy link
Contributor

@feelepxyz feelepxyz left a comment

Choose a reason for hiding this comment

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

🙌

@feelepxyz feelepxyz merged commit 139de1a into dependabot:main Jun 23, 2021
@feelepxyz feelepxyz mentioned this pull request Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants