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.

Sign up for GitHub

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

Jump to bottom

Add verbose information to version generation logic #844

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add verbose information to version generation logic #844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
305b0c1
Add verbose information to version generation logic
tomasz-pankowski-allegro Oct 16, 2024
bfa18e2
fix doc
tomasz-pankowski-allegro Oct 16, 2024
0afbae0
change info level to quiet
tomasz-pankowski-allegro Oct 16, 2024
a3a0847
use debug logging level instead of verbose flag
tomasz-pankowski-allegro Oct 18, 2024
9fb2666
clean up unimportant changes
tomasz-pankowski-allegro Oct 18, 2024
5071c5c
clean up unimportant changes
tomasz-pankowski-allegro Oct 18, 2024
2d8726d
refactor finding latest version
tomasz-pankowski-allegro Oct 18, 2024
c4ba148
after refactor inline logic of finding version number
tomasz-pankowski-allegro Oct 18, 2024
621bb85
lifecycle is the default logging level, use it instead of debug
tomasz-pankowski-allegro Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions src/main/java/pl/allegro/tech/build/axion/release/domain/VersionSorter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package pl.allegro.tech.build.axion.release.domain;

import com.github.zafarkhaja.semver.Version;
import org.gradle.api.logging.Logger;
import org.gradle.api.logging.Logging;
import pl.allegro.tech.build.axion.release.domain.scm.TaggedCommits;
import pl.allegro.tech.build.axion.release.domain.scm.TagsOnCommit;

Expand All @@ -21,6 +23,7 @@
* * any highest (stable or alpha)
*/
class VersionSorter {
private static final Logger logger = Logging.getLogger(VersionSorter.class);

Result pickTaggedCommit(
TaggedCommits taggedCommits,
Expand All @@ -32,9 +35,11 @@ Result pickTaggedCommit(
Set<Version> versions = new LinkedHashSet<>();
LinkedHashMap<Version, Boolean> isVersionNextVersion = new LinkedHashMap<>();
LinkedHashMap<Version, TagsOnCommit> versionToCommit = new LinkedHashMap<>();
boolean tagsFound = false;

for (TagsOnCommit tagsEntry : taggedCommits.getCommits()) {
List<String> tags = tagsEntry.getTags();
tagsFound = tagsFound || !tags.isEmpty();
Copy link

Choose a reason for hiding this comment

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

When this will happen? Since we are iterating through taggedCommits I would assume that each commit has at least 1 tag otherwise it would NOT be tagged 🤔

Copy link
Author

Choose a reason for hiding this comment

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

I think that currently in the code there is actually no way for this class to have an empty tag collection, but there is also nothing in this class that would prevent the creation of such an instance. This class even has a special method for such an occasion ;) TagsOnCommit.empty() which creates an instance with an empty list of tags.
This code, I hope, will work correctly in both cases:

  • if taggedCommits.getCommits() is empty tagsFound==false
  • if the list of tags in taggedCommits.getCommits() is empty then tagsFound==false
    The advantage is that we don't have to wonder if someone really thought it was a great idea to pass an empty list to the TagsOnCommit constructor.
    You can also set a contract for TagsOnCommit, the list of tags cannot be and then we throw an exception otherwise. Then you can easily simplify this code as you say.
    But I'm not insisting, if you feel that this is an unnecessary complication then I'll let you convince me.

Copy link

Choose a reason for hiding this comment

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

Yeah but TaggedCommits != TagsOnCommit. So either name of the variable is wrong or the logic for filling the TaggedCommits class is wrong. But if TaggedCommits does not contain the check for problematic it would be nice to have it there.

I personally would not check for it like this, but I am not insisting on it.


// next version should be ignored when tag is on head
// and there are other, normal tags on it
Expand All @@ -47,10 +52,13 @@ Result pickTaggedCommit(
for (String tag : tags) {
boolean isNextVersion = nextVersionTagPattern.matcher(tag).matches();
if (isNextVersion && (ignoreNextVersionTags || ignoreNextVersionOnHead)) {
logger.lifecycle("Ignoring tag: {}, because it's a next version tag and it's not forced", tag);
continue;
}

Version version = versionFactory.versionFromTag(tag);
logger.lifecycle("Detected version: {} from tag: {}", version, tag);

boolean versionDidNotExist = versions.add(version);
boolean isNormalVersion = !isNextVersion;
// normal tags have precedence over nextVersion tags with same version
Expand All @@ -68,16 +76,16 @@ Result pickTaggedCommit(

}
}


List<Version> versionList = new ArrayList<>(versions);
Collections.sort(versionList, Collections.reverseOrder());

Version version = (versionList.isEmpty() ? versionFactory.initialVersion() : versionList.get(0));
if (version == null) {
version = versionFactory.initialVersion();
if (!tagsFound) {
logger.lifecycle("No tags were found in git history");
}

Version version = versions.stream()
.filter(Objects::nonNull)
.max(Comparator.naturalOrder()) // find latest (max) version
.orElseGet(versionFactory::initialVersion); // if no versions found, use initial version
logger.lifecycle("Version: {}, was selected from versions: {}", version, versions);

TagsOnCommit versionCommit = versionToCommit.get(version);

return new Result(
Expand Down
2 changes: 1 addition & 1 deletion src/test/groovy/pl/allegro/tech/build/axion/release/domain/scm/ScmPropertiesBuilder.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class ScmPropertiesBuilder {
true,
['main', 'master'] as Set,
false,
true
true
)
}

Expand Down