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 verbose information to version generation logic #844

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

Conversation

tomasz-pankowski-allegro
Copy link

@tomasz-pankowski-allegro tomasz-pankowski-allegro commented Oct 16, 2024

Fix for #826

Adding information about missing tags

@tomasz-pankowski-allegro tomasz-pankowski-allegro marked this pull request as ready for review October 16, 2024 14:51
@@ -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

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

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

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

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?

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

@@ -47,10 +57,16 @@ Result pickTaggedCommit(
for (String tag : tags) {
boolean isNextVersion = nextVersionTagPattern.matcher(tag).matches();
if (isNextVersion && (ignoreNextVersionTags || ignoreNextVersionOnHead)) {
if (verbose) {
logger.quiet("Ignoring tag: {}, because it's a next version tag and it's not forced", tag);

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

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


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 🤔

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.

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.35%. Comparing base (099f892) to head (621bb85).
Report is 9 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #844      +/-   ##
============================================
- Coverage     63.74%   62.35%   -1.40%     
- Complexity      442      445       +3     
============================================
  Files            82       83       +1     
  Lines          1633     1684      +51     
  Branches        161      161              
============================================
+ Hits           1041     1050       +9     
- Misses          523      566      +43     
+ Partials         69       68       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@@ -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

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.

4 participants