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

Add support for jgit log search for latest commit when path is given. #422

Merged
merged 8 commits into from
Oct 18, 2021

Conversation

shashken
Copy link
Contributor

@shashken shashken commented Oct 6, 2021

Add support for jgit log search for latest commit when path is given.
Solved an edge case where the latest found was actually one commit prior to the latest commit (the merge to master)
Tries to solve #332
@bgalek - please review
@farrukhnajmi FYI

Solved an edge case where the latest found was actually one commit prior to the latest commit (the merge to master)
@bgalek
Copy link
Member

bgalek commented Oct 6, 2021

Hi, looks fine, but we're gonna need some tests, could you add some?

@shashken
Copy link
Contributor Author

shashken commented Oct 6, 2021

@bgalek I'm not familiar with the tests in this project, could you maybe help me out here?

@bgalek
Copy link
Member

bgalek commented Oct 6, 2021

@shashken sure! look here https://github.com/allegro/axion-release-plugin/tree/master/src/integration/groovy/pl/allegro/tech/build/axion/release
some tests already use CurrentVersion task so it should be easy to setup few tests based on that ;)

…monorepo case and introduced two tests to catch those behaviors
@shashken
Copy link
Contributor Author

shashken commented Oct 7, 2021

@bgalek I implemented two tests to catch 2 cases for the last position, and implemented a new way to choose between lastCommit and the nearest merge.
Can you please review?

@shashken
Copy link
Contributor Author

shashken commented Oct 7, 2021

hmm I thought of something, I think the whole approach for monorepo with excludeDirs was wrong to begin with,
Take this example case:
A (Head)
|
B (Tag 1.2.0)
|
C
|
D
|
E (latest change found for path)
|
F (no change to path)
|
G (Tag 1.1.0)

what we expect:
A (1.2.0)
B-E (1.1.0-snapshot)
F-G (1.1.0)

what is currently happening:
A (1.2.0-snapshot)
B (1.2.0-snapshot)
C-G( 1.1.0-snapshot)

This happens because we check if latestChange == tag, but we dont have to use this approach, instead we can go
Diffformatter.setPathFilter(path);
Diffformatter.scan(latestChange, taggedCommit)

@bgalek
Copy link
Member

bgalek commented Oct 11, 2021

I'll try to look at it today

@shashken
Copy link
Contributor Author

@bgalek I implemented a new logic for monorepo versioning in versionResolver.
If its a monorepo case (path given) it checks for git differences between latestchange and found tag version commit to determine if its a snapshot version.
Added several tests for those behaviours.
The previous fix I proposed is irrelevant here, as this is a completely different approach to determine snapshot versioning for monorepos.
WTYT?

@shashken
Copy link
Contributor Author

@bgalek tag me here if you have any comments/suggestions

@shashken
Copy link
Contributor Author

@bgalek Reminder :) I used a local version of the plugin and it seems to solve the snapshot issue completely

@bgalek
Copy link
Member

bgalek commented Oct 12, 2021

@shashken I gave it a quick look, overall seems ok, I'would change some formatting and names, but we can do it later, please give me little bit more time to think about the solution - but I think it's gonna be ok! we should also update docs :)

@shashken
Copy link
Contributor Author

@bgalek I committed some formatting and naming changes. better now? also what do you want to change in docs? the usage of the plugin remains the same :)
Do you think we can merge this until the beginning of next week maybe?

@bgalek
Copy link
Member

bgalek commented Oct 14, 2021

@shashken cool! Sure, let's say monday - I'll also review some more PR's till then!

@shashken
Copy link
Contributor Author

@bgalek let me know what you want me to change, I'm available today

@bgalek
Copy link
Member

bgalek commented Oct 18, 2021

I'll prepare a release, as promised! :)


when:
VersionContext version = resolver.resolveVersion(versionRules, tagRules, nextVersionRules)
System.out.println("asdf")
Copy link
Member

Choose a reason for hiding this comment

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

@shashken debug left ;)

Copy link
Contributor Author

@shashken shashken Oct 18, 2021

Choose a reason for hiding this comment

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

removed @bgalek

@shashken shashken requested a review from bgalek October 18, 2021 11:42
Copy link
Member

@bgalek bgalek left a comment

Choose a reason for hiding this comment

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

LGTM, thx for your contribution!

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