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 8 commits into
base: main
Choose a base branch
from
+17 −9
Open

Add verbose information to version generation logic #844

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 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
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
5 changes: 5 additions & 0 deletions docs/configuration/version.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,3 +425,8 @@ property:
scmVersion {
sanitizeVersion.set(false)
}

## Troubleshooting
In case of problems you can try to use verbose mode:

#./gradlew release -Prelease.verbose
tomasz-pankowski-allegro marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 5 additions & 0 deletions src/main/groovy/pl/allegro/tech/build/axion/release/domain/VersionConfig.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,12 @@ abstract class VersionConfig extends BaseExtension {
private static final String VERSION_CREATOR_PROPERTY = 'release.versionCreator'
private static final String RELEASE_ONLY_ON_RELEASE_BRANCHES_PROPERTY = 'release.releaseOnlyOnReleaseBranches'
private static final String RELEASE_BRANCH_NAMES_PROPERTY = 'release.releaseBranchNames'
private static final String RELEASE_VERBOSE = 'release.verbose'

@Inject
VersionConfig(Directory repositoryDirectory) {
getDryRun().convention(gradlePropertyPresent(DRY_RUN_FLAG).orElse(false))
getVerbose().convention(gradlePropertyPresent(RELEASE_VERBOSE).orElse(false))
getLocalOnly().convention(false)
getIgnoreUncommittedChanges().convention(true)
getUseHighestVersion().convention(false)
Expand Down Expand Up @@ -81,6 +83,9 @@ abstract class VersionConfig extends BaseExtension {
@Internal
abstract Property<Boolean> getDryRun()

@Internal
abstract Property<Boolean> getVerbose()

@Internal
abstract Property<Boolean> getIgnoreUncommittedChanges()

Expand Down
3 changes: 2 additions & 1 deletion ...ovy/pl/allegro/tech/build/axion/release/infrastructure/config/ScmPropertiesFactory.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ class ScmPropertiesFactory {
config.getUnshallowRepoOnCI().get(),
config.getReleaseBranchNames().get(),
config.getReleaseOnlyOnReleaseBranches().get(),
config.getIgnoreGlobalGitConfig().get()
config.getIgnoreGlobalGitConfig().get(),
config.verbose.get()
)
}
}
3 changes: 2 additions & 1 deletion ...ovy/pl/allegro/tech/build/axion/release/infrastructure/di/VersionResolutionContext.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ class VersionResolutionContext {
this.scmProperties = scmProperties
this.localOnlyResolver = localOnlyResolver
this.versionService = new VersionService(new VersionResolver(scmRepository,
scmProperties.directory.toPath().relativize(projectRoot.toPath()).toString()))
scmProperties.directory.toPath().relativize(projectRoot.toPath()).toString(),
scmProperties.isVerbose()))
}

static VersionResolutionContext create(VersionConfig versionConfig, Directory projectDirectory) {
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/pl/allegro/tech/build/axion/release/domain/VersionResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ public class VersionResolver {
*/
private final String projectRootRelativePath;

public VersionResolver(ScmRepository repository, String projectRootRelativePath) {
public VersionResolver(ScmRepository repository, String projectRootRelativePath, boolean verbose) {
this.repository = repository;
this.projectRootRelativePath = projectRootRelativePath;
this.sorter = new VersionSorter();
this.sorter = new VersionSorter(verbose);
}

public VersionContext resolveVersion(VersionProperties versionProperties, TagProperties tagProperties, NextVersionProperties nextVersionProperties) {
Expand Down
40 changes: 33 additions & 7 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,12 @@
* * any highest (stable or alpha)
*/
class VersionSorter {
private static final Logger logger = Logging.getLogger(VersionSorter.class);
private final boolean verbose;

public VersionSorter(boolean verbose) {
this.verbose = verbose;
}

Result pickTaggedCommit(
TaggedCommits taggedCommits,
Expand All @@ -32,9 +40,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 +57,16 @@ Result pickTaggedCommit(
for (String tag : tags) {
boolean isNextVersion = nextVersionTagPattern.matcher(tag).matches();
if (isNextVersion && (ignoreNextVersionTags || ignoreNextVersionOnHead)) {
if (verbose) {
Copy link

Choose a reason for hiding this comment

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

Instead of using IFs we should just do logger.debug()
https://docs.gradle.org/current/userguide/logging.html

Copy link
Author

Choose a reason for hiding this comment

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

yes, I was thinking about it, but either we'll have to enable debug everywhere, which will probably clutter the logs a lot, but maybe that's ok? or we'll have to indicate for which class we want to change the logging level, which is also probably doable, but maybe less user friendly? What do you think?

Copy link

Choose a reason for hiding this comment

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

but what verbose actually means? Some random google definition:

using or expressed in more words than are needed.

So I would not be worried about that. Also currently there seems to be only 4 more debug logs:
https://github.com/search?q=repo%3Aallegro%2Faxion-release-plugin%20logger.debug&type=code

Copy link
Author

Choose a reason for hiding this comment

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

I'm more afraid of debug logs from the entire gradel tool + everything else we call at the same time. But I haven't checked what the result would look like now with debug turned on, I can check it so we don't guess

Copy link
Author

Choose a reason for hiding this comment

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

I did fast test number o lines:
./gradlew release -Prelease.dryRun -d > debug.log
6877 debug.log

./gradlew release -Prelease.dryRun > standart.log
27 standart.log

Copy link
Author

Choose a reason for hiding this comment

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

but maybe it's still ok, maybe it's better than complicating the code, and if someone needs additional information, they will somehow dig through these thousands of lines of logs?

Copy link
Author

Choose a reason for hiding this comment

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

I did a quick research and checked in a few other plugins if someone uses the verbose flag or rather the login level. From what I see, however, the login level is used, so I will choose this option. It will be much much simpler

logger.quiet("Ignoring tag: {}, because it's a next version tag and it's not forced", tag);
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure which login level would be most appropriate

Copy link
Member

Choose a reason for hiding this comment

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

quiet is default in gradle

Copy link
Collaborator

@radoslaw-panuszewski radoslaw-panuszewski Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No, the lifecycle log level is the default log level in Gradle (docs). If you log on quiet, it will be visible even when running with ./gradlew -q. It's not a good practice to log everything on quiet since users that explicitly want to hide the logs via the -q switch will see them anyway.

Copy link
Member

Choose a reason for hiding this comment

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

my mistake, I forgot about lifecycle

}
continue;
}

Version version = versionFactory.versionFromTag(tag);
if (verbose) {
logger.quiet("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,14 +84,13 @@ Result pickTaggedCommit(

}
}
if (!tagsFound) {
logger.quiet("No tags were found in git history");
Copy link

Choose a reason for hiding this comment

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

imho this should be warning

}


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();
Version version = findBestVersion(versionFactory, versions);
if (verbose) {
logger.quiet("Version: {}, was selected from versions: {}", version, versions);
}

TagsOnCommit versionCommit = versionToCommit.get(version);
Expand All @@ -88,6 +103,17 @@ Result pickTaggedCommit(
);
}

private static Version findBestVersion(VersionFactory versionFactory, Set<Version> versions) {
List<Version> versionList = new ArrayList<>(versions);
versionList.sort(Collections.reverseOrder());

if (versionList.isEmpty()) {
return versionFactory.initialVersion();
}
Version version = versionList.get(0);
return version != null ? version : versionFactory.initialVersion();
}

static class Result {
final Version version;
final boolean isNextVersion;
Expand Down
9 changes: 8 additions & 1 deletion src/main/java/pl/allegro/tech/build/axion/release/domain/scm/ScmProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public class ScmProperties {
private final Set<String> releaseBranchNames;
private final boolean releaseOnlyOnReleaseBranches;
private final boolean ignoreGlobalGitConfig;
private final boolean verbose;

public ScmProperties(
String type,
Expand All @@ -35,7 +36,8 @@ public ScmProperties(
Boolean unshallowRepoOnCI,
Set<String> releaseBranchNames,
boolean releaseOnlyOnReleaseBranches,
boolean ignoreGlobalGitConfig
boolean ignoreGlobalGitConfig,
boolean verbose
) {
this.type = type;
this.directory = directory;
Expand All @@ -51,6 +53,7 @@ public ScmProperties(
this.releaseBranchNames = releaseBranchNames;
this.releaseOnlyOnReleaseBranches = releaseOnlyOnReleaseBranches;
this.ignoreGlobalGitConfig = ignoreGlobalGitConfig;
this.verbose = verbose;
}

public ScmPushOptions pushOptions() {
Expand Down Expand Up @@ -112,4 +115,8 @@ public boolean isReleaseOnlyOnReleaseBranches() {
public boolean isIgnoreGlobalGitConfig() {
return ignoreGlobalGitConfig;
}

public boolean isVerbose() {
return verbose;
}
}
2 changes: 1 addition & 1 deletion ...test/groovy/pl/allegro/tech/build/axion/release/domain/VersionResolverMonoRepoTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class VersionResolverMonoRepoTest extends RepositoryBasedTest {
String secondaryDir = "subProjectSecondary"

def setup() {
resolver = new VersionResolver(repository, subDir)
resolver = new VersionResolver(repository, subDir, false)
commitPrimaryDir("init_file")
}

Expand Down
2 changes: 1 addition & 1 deletion ...est/groovy/pl/allegro/tech/build/axion/release/domain/VersionResolverSubfolderTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class VersionResolverSubfolderTest extends RepositoryBasedTest {

def setup() {
this.projectRootSubfolder = "gradleProjectRoot"
resolver = new VersionResolver(repository, projectRootSubfolder)
resolver = new VersionResolver(repository, projectRootSubfolder, false)
}

def "should return default previous and current version when no tag in repository"() {
Expand Down
2 changes: 1 addition & 1 deletion src/test/groovy/pl/allegro/tech/build/axion/release/domain/VersionResolverTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class VersionResolverTest extends RepositoryBasedTest {
VersionProperties defaultVersionRules = versionProperties().build()

def setup() {
resolver = new VersionResolver(repository, "")
resolver = new VersionResolver(repository, "", false)
}

def "should return default previous and current version when no tag in repository"() {
Expand Down
2 changes: 1 addition & 1 deletion src/test/groovy/pl/allegro/tech/build/axion/release/domain/VersionSorterTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import static pl.allegro.tech.build.axion.release.TagPrefixConf.fullPrefix
*/
class VersionSorterTest extends Specification {

private final VersionSorter sorter = new VersionSorter()
private final VersionSorter sorter = new VersionSorter(false)

private final VersionFactory factory = new VersionFactory(
VersionPropertiesBuilder.versionProperties().build(),
Expand Down
3 changes: 2 additions & 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,8 @@ class ScmPropertiesBuilder {
true,
['main', 'master'] as Set,
false,
true
true,
false
)
}

Expand Down