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

Implement dependency set feature in lein-monolith #93

Merged
merged 21 commits into from
Oct 3, 2023

Conversation

DCardenasAmp
Copy link
Contributor

We'd like to implement support for dependency sets in lein-monolith to help us cleanly cutover projects to newer versions of our managed dependencies.

To this end, this PR implements support for the following:

  • Defining dependency sets in the metaproject file
  • Opting into a dependency set in a child project using :monolith/dependency-set
    • Opting into a dependency set merges in a profile with :managed-dependencies populated from the dependency set
    • This profile is merged before any of the inherited profiles
    • :monolith/dependency-set can be used independently of :monolith/inherit

I added some unit tests to ensure the managed dependencies (and their order) are set correctly when a dependency set is used. I also did some local testing with projects to check that dependencies from a dependency set are passed onto the generated pom file.

@@ -1,19 +1,22 @@
(defproject lein-monolith.example/all "MONOLITH"
:description "Overarching example project."

:pedantic? :ranges
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unit tests seemed to throw a :abort and ranges have a type mismatch merging profiles error before this. From what I could tell :ranges should already be default here but I could just be misunderstanding the real problem here 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh... this definitely shouldn't be required.

Copy link

@kris-preza kris-preza Sep 27, 2023

Choose a reason for hiding this comment

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

Seeing the same thing, it should be defaulting to :ranges 🤷

@DCardenasAmp DCardenasAmp marked this pull request as ready for review September 27, 2023 17:52
@DCardenasAmp DCardenasAmp requested review from greglook and a team September 27, 2023 17:52
Copy link

@kris-preza kris-preza left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@greglook greglook left a comment

Choose a reason for hiding this comment

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

Great, I'm glad this turned out pretty simple to implement. A handful of comments.

doc/config.md Outdated Show resolved Hide resolved
@@ -1,19 +1,22 @@
(defproject lein-monolith.example/all "MONOLITH"
:description "Overarching example project."

:pedantic? :ranges
Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh... this definitely shouldn't be required.

src/lein_monolith/plugin.clj Outdated Show resolved Hide resolved
src/lein_monolith/plugin.clj Outdated Show resolved Hide resolved
src/lein_monolith/plugin.clj Outdated Show resolved Hide resolved
src/lein_monolith/plugin.clj Outdated Show resolved Hide resolved
src/lein_monolith/plugin.clj Outdated Show resolved Hide resolved
Comment on lines 290 to 294
(update subproject :profiles merge
(plugin/build-inherited-profiles
monolith subproject))]))
(into {}
(plugin/build-profiles
monolith subproject)))]))
subprojects)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open question: should we care about profile ordering for fingerprinting? I'm biasing toward keeping the existing behavior of not guaranteeing the order is deterministic for now but I'm open to addressing this here while I'm at it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like it's probably okay to not have ordering matter here, we're just adding the information so the fingerprinting code can see all of the relevant information by including profiles.

Copy link
Collaborator

@greglook greglook left a comment

Choose a reason for hiding this comment

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

Looking good! A few more small optimizations, then will be ready to go.

src/lein_monolith/plugin.clj Outdated Show resolved Hide resolved
src/lein_monolith/plugin.clj Outdated Show resolved Hide resolved
src/lein_monolith/plugin.clj Outdated Show resolved Hide resolved
Comment on lines 290 to 294
(update subproject :profiles merge
(plugin/build-inherited-profiles
monolith subproject))]))
(into {}
(plugin/build-profiles
monolith subproject)))]))
subprojects)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like it's probably okay to not have ordering matter here, we're just adding the information so the fingerprinting code can see all of the relevant information by including profiles.

@DCardenasAmp DCardenasAmp requested review from greglook and removed request for greglook September 28, 2023 23:46
Copy link
Collaborator

@greglook greglook left a comment

Choose a reason for hiding this comment

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

Nice, looks good! 🙆‍♂️

@DCardenasAmp DCardenasAmp merged commit 8cdd9d2 into main Oct 3, 2023
4 checks passed
@DCardenasAmp DCardenasAmp deleted the dcardenas-named-dependencies branch October 3, 2023 16:12
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.

3 participants