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

Predictable artifact order #58

Merged
merged 8 commits into from
Dec 5, 2023

Conversation

jvierling
Copy link
Contributor

On my system the integration test fails (at commit ea21cfe), because the order of the artifacts in the generated lockfile changes slightly. This pull request makes the order of the artifacts more predictable by ordering artifacts alphabetically. The pull request also includes some clean up commits.


public Map<String, MavenArtifact> getDependencies() {
return new HashMap<>(dependencies);
this.dependencies = new TreeMap<>(dependencies);
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe add a comment explaining why it needs to be a TreeMap?

Did I have any test in this repo?
I can't remember :)

@fzakaria
Copy link
Owner

fzakaria commented Dec 1, 2023

LGTM but please add a comment so we don't lose out why it needs to be a TreeMap.
is there a simple test that can be written to validate they output in order?
If it's too much of a hurdle I wouldn't worry about it.

Thank you for the contribution.

@jvierling
Copy link
Contributor Author

@fzakaria Thanks for the review. I have added a comment and a unit test that shows that the artifacts are ordered alphabetically. The integration test Maven2nixIT also shows that the artifacts now appear in alphabetic order (see the modifications of the test's expected lockfile).

@fzakaria
Copy link
Owner

fzakaria commented Dec 4, 2023

LGTM -- I will spend some time Monday or this week to try and update the GitHub actions and validate it passes.
LMK if you are interested in being a maintainer; I haven't really been maintaining this repository at all.

@jvierling
Copy link
Contributor Author

FYI I fixed the Github-Actions on my fork (see https://github.com/jvierling/mvn2nix) by removing the cachix/cachix-action, updating the version of the cachix/install-nix-action, and removing the install_url from the flake jobs.

@fzakaria
Copy link
Owner

fzakaria commented Dec 5, 2023

Do you want to include the updated github action as part of the PR?
or you can make it a separate PR.

@jvierling
Copy link
Contributor Author

I'll include the update to the Github Actions as part of the PR. However, I am not sure if removing cachix/cachix-action is an option for you (I removed it on my fork because I do not have a cachix cache).

@fzakaria fzakaria merged commit e27562c into fzakaria:master Dec 5, 2023
4 checks passed
@fzakaria
Copy link
Owner

fzakaria commented Dec 5, 2023

Thank you for the great PR.

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.

2 participants