Whirr
  1. Whirr
  2. WHIRR-675

Convert all whirr.env.* environment variable labels to upper case

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.8.2
    • Component/s: None
    • Labels:
      None

      Description

      This allows the java properties file and shell environment variables conventions to be maintained. Eg

      whirr.env.repo=cdh4
      whirr.env.repocm=cm4
      whirr.env.repo_host=10.177.196.145/tmph3l7m2vv103/cloudera-repos

      converts to

      REPO=cdh4
      REPOCM=cm4
      REPO_HOST=10.177.196.145/tmph3l7m2vv103/cloudera-repos

      This will remove common configuration errors including ones in the existing codebase

      1. WHIRR-675_VERSION_2.patch
        1 kB
        Graham Gear
      2. WHIRR-675_VERSION_1.patch
        0.8 kB
        Graham Gear

        Issue Links

          Activity

          Hide
          Tom White added a comment -

          I just committed this. Thanks Graham!

          Show
          Tom White added a comment - I just committed this. Thanks Graham!
          Hide
          Steve Loughran added a comment -

          +1, looks good to me

          Show
          Steve Loughran added a comment - +1, looks good to me
          Hide
          Graham Gear added a comment -

          Attached impl that hardcodes Locale.US to avoid unexpected toUpperCase behaviour

          https://issues.apache.org/jira/secure/attachment/12551181/WHIRR-675_VERSION_2.patch

          Show
          Graham Gear added a comment - Attached impl that hardcodes Locale.US to avoid unexpected toUpperCase behaviour https://issues.apache.org/jira/secure/attachment/12551181/WHIRR-675_VERSION_2.patch
          Hide
          Steve Loughran added a comment -

          general practise in ASF code is just to spec US locale for case confert

          String.toUpperCase(Locale.EN_US)
          
          Show
          Steve Loughran added a comment - general practise in ASF code is just to spec US locale for case confert String .toUpperCase(Locale.EN_US)
          Hide
          Graham Gear added a comment -

          Good point, although given the WHIRR-675_VERSION_1.patch implementation assumed the default JVM locale, perhaps it is suffcient to allow this to be set by the standard means available and not introduce any locale specific porcessing? Eg.

          • Set OS/Environment/Java Console Locale
          • Set as part of the whirr JVM command line system properties (WHIRR_CLI_OPTS)

          The other option would involve defining a locale in the whirr configuration and setting explicitly in the bootstrap code of whirr (Locale.setDefault) and ignoring the system set Locale properties.

          I prefer the first option, which feels less obtrusive and invovles a smaller footprint patch.

          What do we think?

          Show
          Graham Gear added a comment - Good point, although given the WHIRR-675 _VERSION_1.patch implementation assumed the default JVM locale, perhaps it is suffcient to allow this to be set by the standard means available and not introduce any locale specific porcessing? Eg. Set OS/Environment/Java Console Locale Set as part of the whirr JVM command line system properties (WHIRR_CLI_OPTS) The other option would involve defining a locale in the whirr configuration and setting explicitly in the bootstrap code of whirr (Locale.setDefault) and ignoring the system set Locale properties. I prefer the first option, which feels less obtrusive and invovles a smaller footprint patch. What do we think?
          Hide
          Steve Loughran added a comment -

          -1 -all Java case conversions should specify a locale for consistent behaviour round the world

          Show
          Steve Loughran added a comment - -1 -all Java case conversions should specify a locale for consistent behaviour round the world
          Show
          Graham Gear added a comment - Attached implementation: https://issues.apache.org/jira/secure/attachment/12550843/WHIRR-675_VERSION_1.patch

            People

            • Assignee:
              Graham Gear
              Reporter:
              Graham Gear
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development