Skip to content

Conversation

@thomasbonset
Copy link

Hello Mr Flower,
Thanks for your plugin, it's really helpful for our software factory.
I needed to amend it a little bit, to classify my module tags into directories in git. So i added this option in the code. Set by default as "-" to avoid regressions as possible as it could be. (mvn tests ok by the way)
I wonder if you could take a look at it anytime.
regards
Thomas Bonset

(cherry picked from commit a293c91a09fe7f27e33027554cbab9f2b7464bf3)
.buildFromCurrentDir();
ResolverWrapper resolverWrapper = new ResolverWrapper(factory, artifactResolver, remoteRepositories, localRepository);
Reactor reactor = Reactor.fromProjects(log, repo, project, projects, buildNumber, modulesToForceRelease, noChangesAction, resolverWrapper, versionNamer);
Reactor reactor = Reactor.fromProjects(log, repo, project, projects, buildNumber, modulesToForceRelease, noChangesAction, resolverWrapper, versionNamer, tagNameSeparator);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not ideal to make an excessively long list of parameters even longer. I have an upcoming PR that fixes this and I would appreciated if you could hold back this PR until then.

Copy link
Author

Choose a reason for hiding this comment

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

Of course, first come first served ! As soon as your PR is approved and merged, il will rebase and adapt mine.

public String arguments;

/**
* <p>Determines the separator between artifactId and groupId<p>
Copy link
Owner

Choose a reason for hiding this comment

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

I think this documentation needs more detail about what this is for and what tags look like, which is {artifactid}-{version} (doesn't actually have groupid). Maybe this should actually be a string like {artifactid}-{version} and the code just does a string replace on {artifactid} and {version}. There could be {groupid} too.

Copy link
Author

@thomasbonset thomasbonset Oct 10, 2019

Choose a reason for hiding this comment

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

Ok I will try to do that. That's what i will implement :
if my custom tag name parameter contains at least substrings "{artifactId}" and "{version}" (minimum required to guarantee uniqueness of a git tag) then do a

tagName=customTagName
        		.replace("{artifactId}", project.getArtifactId())
        		.replace("{groupId}", project.getGroupId())
        		.replace("{version}", version.releaseVersion());

else log a warn and use default value {artifactid}-{version}

Copy link
Owner

Choose a reason for hiding this comment

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

Oh wait, would that complicate the tag lookups in the part of the code that guesses the next build number? (It's been a while since I worked on this so can't really remember)

import static scaffolding.GitMatchers.hasTag;
import static scaffolding.MvnRunner.assertArtifactInLocalRepo;

public class InheritedVersionsWithTagNameSeparatorTest {
Copy link
Owner

Choose a reason for hiding this comment

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

This test should be more focused on the actual change. So delete the copied tests around inheritance and rollbacks etc and just test that the tag separator logic is working. Include tests for invalid separators (i.e. things that are not allowed in git tags).

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.

3 participants