10000 Adds version prefix (3.x) by Joaquimmnetto · Pull Request #58 · wooga/atlas-version · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Dec 5, 2024. It is now read-only.

Adds version prefix (3.x) #58

Merged
merged 2 commits into from
Apr 14, 2023
Merged

Adds version prefix (3.x) #58

merged 2 commits into from
Apr 14, 2023

Conversation

Joaquimmnetto
Copy link
Contributor
@Joaquimmnetto Joaquimmnetto commented Apr 4, 2023

Description

Adds new property prefix to VersionPluginExtension that allows the user to edit a which prefix will be used to search for its applications versions. Before, this prefix was hardcoded to "v".

To do so, version-related git operations were centralised in GitVersionRepository. Before they were spread around and duplicated in several different parts of the code. Tag filtering is still done by TagStrategy-derived classes, which are now associated with GitVersionRepository.

This is a breaking change because now the TagStrategy used to generate version codes are now consistent with the tag strategy. used to generate the actual version. Beforehand, the version code TagStrategy was hardcoded, while the version TagStrategy was variable.

Changes

  • ADD Version tag prefix now is a property (previously only 'v')
  • CHANGE IMPROVE Version code generation now consistent with version generation

@Joaquimmnetto Joaquimmnetto self-assigned this Apr 4, 2023
@Joaquimmnetto Joaquimmnetto marked this pull request as ready for review April 12, 2023 09:30
@Joaquimmnetto Joaquimmnetto requested a review from Larusso April 12, 2023 09:30
Copy link
Member
@Larusso Larusso left a comment

Choose a reason for hiding this comment

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

The implementatio looks good to me.
I would like to address just the smaller stuff. I also saw some missing newlines in last line on some files :)

On top of that could you please also mark in the PR description what the purpose of the new GitVersion, GitVersionRepository is for. We talked about it and I understand from the code that you created that to move some code that was repeated in there.
As a last also mark the removal of the marker strategy. It looks like you renamed it that it becomes the prefix strategy. It kind of is true. Just that the usage was different.

@@ -83,6 +83,7 @@ class VersionPlugin implements Plugin<Project> {
def rawValue = VersionPluginConventions.versionCodeOffset.getValue(project)
Integer.parseInt(rawValue.toString())
8000 }))
extension
extension.prefix.convention("v")
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to implement our usual fallback to env/property flow? I see no harm of allowing the prefix to be set via environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is addressed in #60. I tried to make this PR fit the same code style as the current one in atlas version to minimize the changes. Then later I change the style for everyone.

*/
TagStrategy getTagStrategy() {
tagStrategy
Property<String> prefix = objects.property(String)
Copy link
Member

Choose a reason for hiding this comment

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

You should use the same template @Azurelol and I use for setting up gradle properties.
In case of an extension you can just use a groovy public field. But in case of tasks gradle wants the @input to be put on a getter and that the field is marked private. Since we use traits in a lot of places to share these properties and implementations etc we tend to use the same setup everywhere.

private final Property<$TYPE$> $NAME$ = objects.property($TYPE$)

$INPUT_TYPE$
Property<$TYPE$> get$U_NAME$() {
    $NAME$
}

void set$U_NAME$(Provider<$TYPE$> value) {
    $NAME$.set(value)
}

void set$U_NAME$($TYPE$ value) {
    $NAME$.set(value)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also addressed in #60. I have my own template for this as well, I guess we should put stuff together and make some shared templates for the team?

@@ -309,6 +315,10 @@ trait VersionPluginExtension implements BaseSpec {
releaseStage
}

Provider<GitVersionRepository> versionRepo = git.map {
Grgit it -> GitVersionRepository.fromTagStrategy(it, new PrefixTagStrategy(prefix.get(), true))
Copy link
Member

Choose a reason for hiding this comment

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

can we also please use the same style for closueres etc. The parameters Grgit it -> should be on the line before if you need to split the closure in multiple lines.

@Joaquimmnetto Joaquimmnetto merged commit b6555b5 into release/3.x Apr 14, 2023 6B4B
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0