[GitHub] accumulo pull request #285: Fixes ACCUMULO-4555 treat Version as a simple st...

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #285: Fixes ACCUMULO-4555 treat Version as a simple st...

ctubbsii
GitHub user glitch opened a pull request:

    https://github.com/apache/accumulo/pull/285

    Fixes ACCUMULO-4555 treat Version as a simple string

    Fix for ticket ACCUMULO-4555 (one of the newbie tickets ;)

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/glitch/accumulo ACCUMULO-4555

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/accumulo/pull/285.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #285
   
----
commit dd9757de70353d4e36138aad84ea30ab5dd2978d
Author: Kyle <[hidden email]>
Date:   2017-07-28T13:58:46Z

    Fixes ACCUMULO-4555 treat Version as a simple string

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #285: Fixes ACCUMULO-4555 treat Version as a simple string

ctubbsii
Github user ivakegg commented on the issue:

    https://github.com/apache/accumulo/pull/285
 
    LGTM +1


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #285: Fixes ACCUMULO-4555 treat Version as a simple st...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/285#discussion_r130330269
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/util/Version.java ---
    @@ -53,74 +56,13 @@ public void execute(final String[] args) throws Exception {
         System.out.println(runTMP.getField("VERSION").get(null));
       }
     
    -  String package_ = null;
    -  int major = 0;
    -  int minor = 0;
    -  int release = 0;
    -  String etcetera = null;
    -
    -  public Version(String everything) {
    -    parse(everything);
    -  }
    -
    -  private void parse(String everything) {
    -    Pattern pattern = Pattern.compile("(([^-]*)-)?(\\d+)(\\.(\\d+)(\\.(\\d+))?)?(-(.*))?");
    -    Matcher parser = pattern.matcher(everything);
    -    if (!parser.matches()) {
    -      log.warn("Unable to parse '{}' as a version", everything);
    -      return;
    -    }
    -
    -    if (parser.group(1) != null)
    -      package_ = parser.group(2);
    -    major = Integer.parseInt(parser.group(3));
    -    minor = 0;
    -    if (parser.group(5) != null)
    -      minor = Integer.parseInt(parser.group(5));
    -    if (parser.group(7) != null)
    -      release = Integer.parseInt(parser.group(7));
    -    if (parser.group(9) != null)
    -      etcetera = parser.group(9);
    -
    -  }
    -
    -  public String getPackage() {
    -    return package_;
    -  }
    -
    -  public int getMajorVersion() {
    -    return major;
    -  }
    -
    -  public int getMinorVersion() {
    -    return minor;
    -  }
    -
    -  public int getReleaseVersion() {
    -    return release;
    -  }
    -
    -  public String getEtcetera() {
    -    return etcetera;
    +  public Version(String version) {
    --- End diff --
   
    Can probably also delete this constructor, the `toString()` method, and `this.version`, as well as the entire `TestVersion.java`.
   
    Other than the `KeywordExecutable` stuff (to support printing the version from the command-line), this class seems to only be used in one place (server/base/src/main/java/org/apache/accumulo/server/Accumulo.java), and only as an unnecessary wrapper around `Constants.VERSION` for a RuntimeException message.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #285: Fixes ACCUMULO-4555 treat Version as a simple st...

ctubbsii
In reply to this post by ctubbsii
Github user glitch commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/285#discussion_r130338251
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/util/Version.java ---
    @@ -53,74 +56,13 @@ public void execute(final String[] args) throws Exception {
         System.out.println(runTMP.getField("VERSION").get(null));
       }
     
    -  String package_ = null;
    -  int major = 0;
    -  int minor = 0;
    -  int release = 0;
    -  String etcetera = null;
    -
    -  public Version(String everything) {
    -    parse(everything);
    -  }
    -
    -  private void parse(String everything) {
    -    Pattern pattern = Pattern.compile("(([^-]*)-)?(\\d+)(\\.(\\d+)(\\.(\\d+))?)?(-(.*))?");
    -    Matcher parser = pattern.matcher(everything);
    -    if (!parser.matches()) {
    -      log.warn("Unable to parse '{}' as a version", everything);
    -      return;
    -    }
    -
    -    if (parser.group(1) != null)
    -      package_ = parser.group(2);
    -    major = Integer.parseInt(parser.group(3));
    -    minor = 0;
    -    if (parser.group(5) != null)
    -      minor = Integer.parseInt(parser.group(5));
    -    if (parser.group(7) != null)
    -      release = Integer.parseInt(parser.group(7));
    -    if (parser.group(9) != null)
    -      etcetera = parser.group(9);
    -
    -  }
    -
    -  public String getPackage() {
    -    return package_;
    -  }
    -
    -  public int getMajorVersion() {
    -    return major;
    -  }
    -
    -  public int getMinorVersion() {
    -    return minor;
    -  }
    -
    -  public int getReleaseVersion() {
    -    return release;
    -  }
    -
    -  public String getEtcetera() {
    -    return etcetera;
    +  public Version(String version) {
    --- End diff --
   
    The parent ticket ACCUMULO-4554 stated that you might want the ability to set the version for testing and setting it to a non-matching format caused IllegalArgumentException to be thrown. That's why I retained the ability to set an arbitrary version through the constructor.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #285: Fixes ACCUMULO-4555 treat Version as a simple st...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/285#discussion_r130346697
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/util/Version.java ---
    @@ -53,74 +56,13 @@ public void execute(final String[] args) throws Exception {
         System.out.println(runTMP.getField("VERSION").get(null));
       }
     
    -  String package_ = null;
    -  int major = 0;
    -  int minor = 0;
    -  int release = 0;
    -  String etcetera = null;
    -
    -  public Version(String everything) {
    -    parse(everything);
    -  }
    -
    -  private void parse(String everything) {
    -    Pattern pattern = Pattern.compile("(([^-]*)-)?(\\d+)(\\.(\\d+)(\\.(\\d+))?)?(-(.*))?");
    -    Matcher parser = pattern.matcher(everything);
    -    if (!parser.matches()) {
    -      log.warn("Unable to parse '{}' as a version", everything);
    -      return;
    -    }
    -
    -    if (parser.group(1) != null)
    -      package_ = parser.group(2);
    -    major = Integer.parseInt(parser.group(3));
    -    minor = 0;
    -    if (parser.group(5) != null)
    -      minor = Integer.parseInt(parser.group(5));
    -    if (parser.group(7) != null)
    -      release = Integer.parseInt(parser.group(7));
    -    if (parser.group(9) != null)
    -      etcetera = parser.group(9);
    -
    -  }
    -
    -  public String getPackage() {
    -    return package_;
    -  }
    -
    -  public int getMajorVersion() {
    -    return major;
    -  }
    -
    -  public int getMinorVersion() {
    -    return minor;
    -  }
    -
    -  public int getReleaseVersion() {
    -    return release;
    -  }
    -
    -  public String getEtcetera() {
    -    return etcetera;
    +  public Version(String version) {
    --- End diff --
   
    This class plays no role in setting the version of Accumulo, for testing or otherwise. You set a version by altering the POM, which populates `Constants.VERSION` during the Maven build. As far as I can tell, the only function it ever served was to produce surprising errors like the one in the parent issue by arbitrarily imposing unnecessary restrictions on what the version string looked like (and now, of course, the `KeywordExecutable` bits plays the role of providing the implementation for `bin/accumulo version` CLI command). Anything that's not part of the `KeywordExecutable` bits can be removed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #285: Fixes ACCUMULO-4555 treat Version as a simple st...

ctubbsii
In reply to this post by ctubbsii
Github user glitch commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/285#discussion_r130347391
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/util/Version.java ---
    @@ -53,74 +56,13 @@ public void execute(final String[] args) throws Exception {
         System.out.println(runTMP.getField("VERSION").get(null));
       }
     
    -  String package_ = null;
    -  int major = 0;
    -  int minor = 0;
    -  int release = 0;
    -  String etcetera = null;
    -
    -  public Version(String everything) {
    -    parse(everything);
    -  }
    -
    -  private void parse(String everything) {
    -    Pattern pattern = Pattern.compile("(([^-]*)-)?(\\d+)(\\.(\\d+)(\\.(\\d+))?)?(-(.*))?");
    -    Matcher parser = pattern.matcher(everything);
    -    if (!parser.matches()) {
    -      log.warn("Unable to parse '{}' as a version", everything);
    -      return;
    -    }
    -
    -    if (parser.group(1) != null)
    -      package_ = parser.group(2);
    -    major = Integer.parseInt(parser.group(3));
    -    minor = 0;
    -    if (parser.group(5) != null)
    -      minor = Integer.parseInt(parser.group(5));
    -    if (parser.group(7) != null)
    -      release = Integer.parseInt(parser.group(7));
    -    if (parser.group(9) != null)
    -      etcetera = parser.group(9);
    -
    -  }
    -
    -  public String getPackage() {
    -    return package_;
    -  }
    -
    -  public int getMajorVersion() {
    -    return major;
    -  }
    -
    -  public int getMinorVersion() {
    -    return minor;
    -  }
    -
    -  public int getReleaseVersion() {
    -    return release;
    -  }
    -
    -  public String getEtcetera() {
    -    return etcetera;
    +  public Version(String version) {
    --- End diff --
   
    Sounds good, I'll gut it some more.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #285: Fixes ACCUMULO-4555 treat Version as a simple string

ctubbsii
In reply to this post by ctubbsii
Github user glitch commented on the issue:

    https://github.com/apache/accumulo/pull/285
 
    Updated & rebased PR based on feedback from @ctubbsii
    Local build passing using:  mvn verify -Psunny


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #285: Fixes ACCUMULO-4555 treat Version as a simple string

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on the issue:

    https://github.com/apache/accumulo/pull/285
 
    +1 on the change, this one just bit me the other day.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #285: Fixes ACCUMULO-4555 treat Version as a simple string

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on the issue:

    https://github.com/apache/accumulo/pull/285
 
    @joshelser Should we apply to previous branches, or just master? Is this biting you in previous release lines?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #285: Fixes ACCUMULO-4555 treat Version as a simple string

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on the issue:

    https://github.com/apache/accumulo/pull/285
 
    > Should we apply to previous branches, or just master? Is this biting you in previous release lines?
   
    I'd put it everywhere. What I'll do sometimes when I have to build upstream components to build downstream (e.g. hadoop and ZK before Accumulo) is something like `mvn versions:set -DnewVersion=x.y.z-josh` for each. This makes sure that I don't accidentally get some other version that doesn't have my patch I'm trying to test.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #285: Fixes ACCUMULO-4555 treat Version as a simple string

ctubbsii
In reply to this post by ctubbsii
Github user milleruntime commented on the issue:

    https://github.com/apache/accumulo/pull/285
 
    I can merge this into master and then apply the changes to 1.7 and 1.8.  
   
    @glitch I don't think you are listed yet under Contributors.  Would you like to be added to the list?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #285: Fixes ACCUMULO-4555 treat Version as a simple string

ctubbsii
In reply to this post by ctubbsii
Github user glitch commented on the issue:

    https://github.com/apache/accumulo/pull/285
 
    @milleruntime Sure that'd be awesome, thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #285: Fixes ACCUMULO-4555 treat Version as a simple string

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on the issue:

    https://github.com/apache/accumulo/pull/285
 
    @glitch If you'd like to be added to https://accumulo.apache.org/people/ , please provide details you want listed here, or perform a pull request to https://github.com/apache/accumulo-website . Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #285: Fixes ACCUMULO-4555 treat Version as a simple string

ctubbsii
In reply to this post by ctubbsii
Github user glitch commented on the issue:

    https://github.com/apache/accumulo/pull/285
 
    https://github.com/apache/accumulo-website/pull/21
    Thanks all!  When I get some more free time, I'll see if I can eliminate another paper cut or tackle something more interesting.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #285: Fixes ACCUMULO-4555 treat Version as a simple string

ctubbsii
In reply to this post by ctubbsii
Github user milleruntime commented on the issue:

    https://github.com/apache/accumulo/pull/285
 
    Back ported changes to 1.7 and merged up.  This PR can be closed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #285: Fixes ACCUMULO-4555 treat Version as a simple st...

ctubbsii
In reply to this post by ctubbsii
Github user asfgit closed the pull request at:

    https://github.com/apache/accumulo/pull/285


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Loading...