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.

Sign up for GitHub

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 support for -Dbuild.snapshot=true/false and default to building snapshot. #1409

Closed
wants to merge 1 commit into from
Closed

Add support for -Dbuild.snapshot=true/false and default to building snapshot. #1409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ jobs:

- name: Build OpenSearch
working-directory: ./OpenSearch
run: ./gradlew publishToMavenLocal -Dbuild.snapshot=false
run: ./gradlew publishToMavenLocal
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes version of OpenSearch and not the security plugin to -SNAPSHOT. It was supposed to be part of #1335. @cliu123 I'd prefer to have a separate PR that changes OpenSearch dependency version to -SNAPSHOT

Copy link
Member Author

Choose a reason for hiding this comment

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

It does both at build time.


- name: Checkstyle
run: mvn -B checkstyle:checkstyle
Expand Down
3 changes: 2 additions & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ buildDir = 'gradle-build'

ext {
securityPluginVersion = '1.1.0.0'
opensearchVersion = System.getProperty("opensearch.version", "1.1.0-SNAPSHOT")
isSnapshot = "true" == System.getProperty("build.snapshot", "true")
}

Expand Down Expand Up @@ -67,7 +68,7 @@ ospackage {
user 'root'
permissionGroup 'root'

requires('opensearch-oss', "1.1.0", EQUAL)
requires('opensearch-oss', opensearchVersion - '-SNAPSHOT', EQUAL)
packager = 'Amazon'
vendor = 'Amazon'
os = 'LINUX'
Expand Down
2 changes: 1 addition & 1 deletion plugin-descriptor.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
description=Provide access control related features for OpenSearch 1.0.0
#
# 'version': plugin's version
version=1.1.0.0-SNAPSHOT
version=1.1.0.0
Copy link
Member

Choose a reason for hiding this comment

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

This is security plugin version, not the OpenSearch version. If the goal of this PR is to add -SNAPSHOT suffix in the OpenSearch version, this line doesn't need to be changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

The plugin version needs to be incremented because we're building OpenSearch 1.1. Every other plugin says it's version 1.1.0.0-SNAPSHOT.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect 1.1.0.0-SNAPSHOT, not 1.1.0.0. If it is not snapshot, version should not be changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

This properties file by default has a number without snapshot. Then, with a profile, we change it to be -SNAPSHOT unless -Dbuild.snapshot=false. You can't have the logical condition the other way around if you want the default to be -Dbuild.snapshot=false.

#
# 'name': the plugin name
name=opensearch-security
Expand Down
15 changes: 14 additions & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
<groupId>org.opensearch</groupId>
<artifactId>opensearch-security</artifactId>
<packaging>jar</packaging>
<version>1.1.0.0-SNAPSHOT</version>
<version>${version}</version>
<name>OpenSearch Security</name>
<description>OpenSearch Security</description>
<url>https://github.com/opensearch-project/security</url>
Expand Down Expand Up @@ -69,6 +69,7 @@
<maven.compiler.release>8</maven.compiler.release>

<opensearch.version>1.1.0</opensearch.version>
<version>1.1.0.0</version>
Copy link
Member

@cliu123 cliu123 Aug 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is referenced on the line 37 that is security plugin version. Same as this comment, doesn't seem to need a change at here. If yes, this is the example PR to bump plugin version.

Copy link
Member Author

Choose a reason for hiding this comment

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

It had to move to properties or you cannot set it via a profile.


<!-- deps -->
<netty-native.version>2.0.25.Final</netty-native.version>
Expand Down Expand Up @@ -831,6 +832,18 @@
</resources>
</build>
<profiles>
<profile>
<activation>
<property>
<name>build.snapshot</name>
<value>!false</value>
</property>
</activation>
<properties>
<version>1.1.0.0-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this PR is bumping security version from 1.0.1.0 to 1.1.0.0. If that's the case, is that the case?

Copy link
Member Author

@dblock dblock Aug 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this is the case, I updated the description of the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Must be consistent with plugin-descriptor.properties, I don't see how this can be done using profiles.

Copy link
Member Author

Choose a reason for hiding this comment

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

Update: by default the version comes from plugin.properties and is set to 1.1.0.0. This sets the version to 1.1.0.0-SNAPSHOT unless you specify -Dbuild.snapshot=false. Meaning it sets it by default. This effectively enables building both, snapshot or not snapshot, with snapshot by default.

If you put 1.1.0.0-SNAPSHOT in the properties file, then this profile condition would have to be inverted. But then the default can't be snapshot=true.

<opensearch.version>1.1.0.0-SNAPSHOT</opensearch.version>
</properties>
</profile>
<profile>
<id>advanced</id>
<build>
Expand Down