-
Notifications
You must be signed in to change notification settings - Fork 3k
API, Build: Explicitly pass version into Git Properties Plugin #12949
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
API, Build: Explicitly pass version into Git Properties Plugin #12949
Conversation
| @Test | ||
| public void testVersionMatchesFile() throws IOException { | ||
| Path versionPath = Paths.get("../version.txt").toAbsolutePath(); | ||
| assumeThat(java.nio.file.Files.exists(versionPath)).isTrue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
If we set the project.version in the system property for tests in build.gradle
test {
systemProperty "project.version", project.version
}
we can always check
assertThat(IcebergBuild.version())
.isEqualTo(System.getProperty("project.version"))
This way we don't need two testcase and also it won't be a conditional test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good suggestion, but I really want to test that file. I'm trying to test against the thing we know must be correct. We basically directly pipe this value into version.txt in our source-release script. If we somehow bungle gradle in the future and that value doesn't match the text file for whatever reason I'd like to know. I can add this test as well as an assert though
ajantha-bhat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The underlying issue is that we automatically bumped the Git Properties plugin which now uses a slightly different mechanism for determining what the version of the project is from before
Thanks for digging into this issue and fixing it 👍
singhpk234
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !
HonahX
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| assertThat(IcebergBuild.version()).isNotEqualTo("unspecified"); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajantha-bhat Added this test, all good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @RussellSpitzer!
|
I'll go ahead and merge, thank you @RussellSpitzer for digging into it, and thanks @ajantha-bhat @singhpk234 @HonahX for reviewing. Thank you @sullis for reporting this issue as well! |
jbonofre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (sorry I'm just waking up :)).
sullis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Fixes #12926
The underlying issue is that we automatically bumped the Git Properties plugin which now uses a slightly different mechanism for determining what the version of the project is from before. To avoid any issues in the future, I decided to just explicitly pass in our custom version function to the plugin.
I then added a few tests to check that we don't have an undefined value and if a version.txt file is present make sure it matches.