Merge lp:~dannf/charms/trusty/elasticsearch/unx86ify into lp:charms/trusty/elasticsearch

[dann frazier]

[Marco Ceppi (marcoceppi)]

LGTM

review: Approve

[Michael Nelson (michael.nelson)]

Hi Marco, Dann. I thought Dann and I had discussed this earlier, but I can't find the record (and old MP?). Generally, I'm not sure that updating the default options with specifics like this is a good idea - people with specific cases can override it as needed (and a mention in the README might be good). Specifically in this case, from what I remember, Dann you'd opened a bug upstream with elastico about this, and they'd explicitly said they weren't testing on this arch:

https://github.com/elastic/elasticsearch/issues/9299

"Closing this as we don;t want to provide packages for architecture which we are not testing. On these architectures Elasticsearch can still be installed from the zip or tar.gz distributions"

[dann frazier (dannf)]

On Mon, Nov 9, 2015 at 12:50 PM, Michael Nelson
<email address hidden> wrote:
> Hi Marco, Dann. I thought Dann and I had discussed this earlier, but I can't find the record (and old MP?).

I don't recall that - but it has been many months, so I wouldn't be
surprised. Apologies if I've forgotten - I definitely did not intend
to dismiss your feedback!

> Generally, I'm not sure that updating the default options with specifics like this is a good idea -
> people with specific cases can override it as needed (and a mention in the README might be good).

One of the goals of charms, aiui, is specifically to hide this type of
complexity. Leaving it up to a user to know that, because they happen
to be deploying on platform $foo, they need to specify config argument
$bar seems like magic--. If a charm can be made to *just work*,
independent of substrate, I'd like to achieve that. That said, I'm not
married to this specific implementation of making things just work -
it just seemed to fit the charm design the best, but I'm open to other
suggestions.

> Specifically in this case, from what I remember, Dann you'd opened a bug upstream with elastico about this, and they'd explicitly said they weren't testing on this arch:
>
> https://github.com/elastic/elasticsearch/issues/9299
>
> "Closing this as we don;t want to provide packages for architecture which we are not testing. On these architectures Elasticsearch can still be installed from the zip or tar.gz distributions"

Which I internalized as "the charm needs to workaround this for the
foreseeable future". That is, *I* know that this charm is functional
on arm64 (we demoed this at ODS last spring), and I know that there
users who *want* to use this charm to evaluate elasticsearch on arm64
hw. Of course, if there is a standard way to warn charm users they are
entering into unsupported land, I'd be all for adding such a warning
here.

  -dann

>
> --
> https://code.launchpad.net/~dannf/charms/trusty/elasticsearch/unx86ify/+merge/246757
> You are the owner of lp:~dannf/charms/trusty/elasticsearch/unx86ify.
>
> Launchpad-Message-Rationale: Owner
> Launchpad-Message-For: dannf
> Launchpad-Notification-Type: code-review
> Launchpad-Branch: ~dannf/charms/trusty/elasticsearch/unx86ify

[Michael Nelson (michael.nelson)]

On Tue, Nov 10, 2015 at 8:37 AM dann frazier <email address hidden>
wrote:

> On Mon, Nov 9, 2015 at 12:50 PM, Michael Nelson
> <email address hidden> wrote:
> > Hi Marco, Dann. I thought Dann and I had discussed this earlier, but I
> can't find the record (and old MP?).
>
> I don't recall that - but it has been many months, so I wouldn't be
> surprised. Apologies if I've forgotten - I definitely did not intend
> to dismiss your feedback!
>
> > Generally, I'm not sure that updating the default options with specifics
> like this is a good idea -
> > people with specific cases can override it as needed (and a mention in
> the README might be good).
>
> One of the goals of charms, aiui, is specifically to hide this type of
> complexity. Leaving it up to a user to know that, because they happen
> to be deploying on platform $foo, they need to specify config argument
> $bar seems like magic--. If a charm can be made to *just work*,
> independent of substrate, I'd like to achieve that. That said, I'm not
> married to this specific implementation of making things just work -
> it just seemed to fit the charm design the best, but I'm open to other
> suggestions.
>

Definitely agree - the charm should just work for all supported platforms...

>
> > Specifically in this case, from what I remember, Dann you'd opened a bug
> upstream with elastico about this, and they'd explicitly said they weren't
> testing on this arch:
> >
> > https://github.com/elastic/elasticsearch/issues/9299
> >
> > "Closing this as we don;t want to provide packages for architecture
> which we are not testing. On these architectures Elasticsearch can still be
> installed from the zip or tar.gz distributions"
>
> Which I internalized as "the charm needs to workaround this for the
> foreseeable future". That is, *I* know that this charm is functional
> on arm64 (we demoed this at ODS last spring), and I know that there
> users who *want* to use this charm to evaluate elasticsearch on arm64
> hw. Of course, if there is a standard way to warn charm users they are
> entering into unsupported land, I'd be all for adding such a warning
> here.
>

Yeah - that's what I'm uncertain about - and that's where a note in the
README for running and evaluating on unsupported arches would be easiest.
But I'm only -0, fine as is, just wanted to be explicit that elastico have
marked the above bug as wont-fix because they don't test on that platform.

Thanks Dann.

=== modified file 'config.yaml'
--- config.yaml	2014-10-31 03:43:31 +0000
+++ config.yaml	2015-01-16 18:01:31 +0000
@@ -1,7 +1,7 @@
 options:
   apt-repository:
     type: string
-    default: "deb http://packages.elasticsearch.org/elasticsearch/1.2/debian stable main"
+    default: "deb [arch=amd64] http://packages.elasticsearch.org/elasticsearch/1.2/debian stable main"
     description: |
       A deb-line for the apt archive which contains the elasticsearch package.
       This is necessary until elasticsearch gets into the debian/ubuntu archives.