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

Staging for version increment automation. #192

Closed
wants to merge 14 commits into from

Conversation

prudhvigodithi
Copy link
Contributor

@prudhvigodithi prudhvigodithi commented Jul 1, 2022

Description

This is the staging PR for to execute workflows that auto raise the version increment PR's
Example: prudhvigodithi#1

Issues Resolved

Part of: opensearch-project/opensearch-build#1375
From solution: opensearch-project/opensearch-build#1375 (comment)
Related issue: #193

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

This was referenced Jul 5, 2022
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Is the change of System.getProperty for project.property really necessary as part of this?

build.gradle Outdated Show resolved Hide resolved
@prudhvigodithi
Copy link
Contributor Author

prudhvigodithi commented Jul 6, 2022

Is the change of System.getProperty for project.property really necessary as part of this?

Hey @dblock
So the right fit for our use case is to use project property, we identify the opensearch.version property, infer the plugin version based on this opensearch.version, and next time when we run ./gradlew tasks we dont have the right opensearch.version based on which the plugin version was inferred (because with existing setup, using -D we only have it during runtime). With -P passing project property, we have opensearch.version always part of the project and next time when we version increment we update this opensearch.version property (which is part of gradle.properties file.).
Note: Also project properties can be overridden with -P ./gradlew assemble --no-daemon --refresh-dependencies -Popensearch.version=$VERSION

This change is necessary (the main change that drives the version increment automation), the idea from the solution is to make sure we maintain a gradle.properties file, this file has all the project properties which can be used for now and for future automation properties as well.

More details

A gradle project property (-P)is a property that can be accessed in project’s build.gradle, in our case we use opensearch.version, build.snapshot, build.version_qualifier just to infer the plugin version which is part of build.gradle,

System properties(-D) are accessed via normal Java/Groovy system properties access methods (works only for runtime injection).
@bbarani @peterzhuamazon

@prudhvigodithi
Copy link
Contributor Author

Hey @dblock I understand your concern, since this -D (system property) is added at multiple places, this PR pushes multiple changes at same time, one to add gradle.properties file, two to add two different tasks, three to make change from system to project property.

@dblock
Copy link
Member

dblock commented Jul 6, 2022

Ok, I think I understand. The real reason is that ant.propertyfile is easier to use than some way of replacing a version inside build.gradle. I think you should push the property change separately, either before, or after the version increment.

If you do it before it might be easier/smaller amount of changes, especially if you need to modify files that aren't just .properties.

task setVersion {
    onlyIf { System.getProperty('newVersion') }
    // replace in build.gradle with whatever file search/replace
    doLast {
        println "Setting version to ${System.getProperty('newVersion')}."
    }
}

@prudhvigodithi
Copy link
Contributor Author

prudhvigodithi commented Jul 6, 2022

Ok, I think I understand. The real reason is that ant.propertyfile is easier to use than some way of replacing a version inside build.gradle. I think you should push the property change separately, either before, or after the version increment.

Yes @dblock without having property change we can still proceed to version increment, We solely rely on opensearch_version = System.getProperty("opensearch.version", "3.0.0-SNAPSHOT") the default value (here its 3.0.0-SNAPSHOT) and replace with version that is out there in OpenSearch core repo
The downside with this is we will not have a fixed project property opensearch.version where we can compare the version against with.
Example version increment PR I have tested https://github.com/prudhvigodithi/sql/pull/8/files
Workflow run: https://github.com/prudhvigodithi/opensearch-build/runs/7224621950?check_suite_focus=true
Task: https://github.com/prudhvigodithi/sql/blob/main/build.gradle#L152-L172

@prudhvigodithi
Copy link
Contributor Author

Also @dblock adding to your point to use onlyIf, we can only skip the execution phase of the task, not its configuration phase, meaning for example when we run ./gradlew properties the task fails with output property newVersion not found, we can use onlyIf condition, but add a fail-safe way to access the project properties, whereas using if is more clean more only runs a task when has -PnewVersion
Could not get unknown property 'newVersion' for task

@prudhvigodithi
Copy link
Contributor Author

prudhvigodithi commented Jul 7, 2022

Hey @dblock Pushed the change in my latest commit, can you please check the task now considering the above inputs.
1st change will only be version increment, hence only added the task versionIncrement, this will use the existing

opensearch_version = System.getProperty("opensearch.version", "1.3.3-SNAPSHOT")

opensearch_version property (here from above line the default is 1.3.3-SNAPSHOT) compares with core and raises a PR.
Thank you
Sample common-utils [AUTO] PR: https://github.com/prudhvigodithi/common-utils/pull/2/files
@bbarani

@prudhvigodithi
Copy link
Contributor Author

Thanks @dblock for all the details, I see now why onlyIF is good use case here. I have pushed the change in my latest commit.

./gradlew versionIncrement
> Task :versionIncrement SKIPPED

./gradlew versionIncrement -DnewVersion=1.3.3-SNAPSHOT
> Task :versionIncrement
Setting version to 1.3.3-SNAPSHOT.

./gradlew build nor ./gradlew build -DnewVersion=1.3.4-SNAPSHOT
(does not run versionIncrement task)

@prudhvigodithi prudhvigodithi marked this pull request as ready for review July 8, 2022 15:30
@prudhvigodithi prudhvigodithi requested a review from a team July 8, 2022 15:30
build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
dblock
dblock previously approved these changes Jul 8, 2022
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Lgtm

praveensameneni
praveensameneni previously approved these changes Jul 8, 2022
@dblock
Copy link
Member

dblock commented Jul 12, 2022

I imagine you're renaming tasks following opensearch-project/opensearch-plugin-template-java#32?

@prudhvigodithi
Copy link
Contributor Author

Ye @dblock I will be shortly pushing the code renaming the task as updateVersion.

@prudhvigodithi
Copy link
Contributor Author

Thanks @dblock I have raised a new PR to main branch with backport labels, please advise if I can close this?
#200
Thank you

@dblock
Copy link
Member

dblock commented Jul 14, 2022

Yep, use backports.

@dblock dblock closed this Jul 14, 2022
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