Skip to content

Update maas_influxdata_key variable#415

Merged
cloudnull merged 1 commit intomasterfrom
mattt/remove_tigkstack
Dec 13, 2017
Merged

Update maas_influxdata_key variable#415
cloudnull merged 1 commit intomasterfrom
mattt/remove_tigkstack

Conversation

@mattt416
Copy link
Copy Markdown
Contributor

@mattt416 mattt416 commented Dec 12, 2017

In #414 we tried making this include conditional, however this playbook
is still being included when run w/ older versions of ansible (1.9.6
as an example).

Instead, we update maas_influxdata_key to have an id and keyserver key.
This allows apt_key to work since it doesn't need to speak to
https://repos.influxdata.com/ directly. Using ansible's apt_key with
https://repos.influxdata.com/ fails because of SNI and:

  1. Prefer the stdlib SSLContext over urllib3 context ansible/ansible#32053 (not backported to
    2.3.2.0)
  2. ansible 1.9.6 doesn't seem to support SNI on Trusty (python < 2.7.9)

Copy link
Copy Markdown
Contributor

@CrashenX CrashenX left a comment

Choose a reason for hiding this comment

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

I don't think we want to remove tigkstack. This is some foundational work @cloudnull did that we will be using as a baseline / reference for Operational Insight. I'll defer to @jedsmith and @cloudnull .

@jedsmith
Copy link
Copy Markdown
Contributor

This change is probably OK, @CrashenX, because it's just preventing the tests from running. The tests still exist, the way I read it. Happy to be shown I'm wrong.

Copy link
Copy Markdown
Contributor

@CrashenX CrashenX left a comment

Choose a reason for hiding this comment

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

I'll unblock. Do we really want tests not to run? AIUI, this IS deployed to customer envs, it's just not enabled by default. Probably want to sync with @npawelek and @cloudnull

@CrashenX CrashenX dismissed their stale review December 12, 2017 17:31

¯_(ツ)_/¯

@mattt416
Copy link
Copy Markdown
Contributor Author

Hold off on merging this for now -- let's switch the apt_key ansible tasks to using shell commands to bypass the bug ... and we can then let this stuff be deployed/tested as it was.

@mattt416 mattt416 changed the title Remove deployment of tigkstack from tests DO NOT MERGE Remove deployment of tigkstack from tests Dec 12, 2017
@claco
Copy link
Copy Markdown
Contributor

claco commented Dec 12, 2017

I can confirm we'd not like to remove it from an RLM perspective, or at least, we have some work in the future to use it during deployments to measure cp downtime, etc.

@tonytan4ever
Copy link
Copy Markdown
Contributor

@mattt416: Just a quick caveat: switching to shell command might break ansible-lint check

@mattt416 mattt416 force-pushed the mattt/remove_tigkstack branch from 317ab25 to 7070ec8 Compare December 12, 2017 18:15
@mattt416 mattt416 changed the title DO NOT MERGE Remove deployment of tigkstack from tests DO NOT MERGE Update Add influxdata apt-keys tasks Dec 12, 2017
@mattt416 mattt416 force-pushed the mattt/remove_tigkstack branch from 39f021d to 1532918 Compare December 12, 2017 18:40
In #414 we tried making this include conditional, however this playbook
is still being included when run w/ older versions of ansible (1.9.6
as an example).

Instead, we update maas_influxdata_key to have an id and keyserver key.
This allows apt_key to work since it doesn't need to speak to
https://repos.influxdata.com/ directly.  Using ansible's apt_key with
https://repos.influxdata.com/ fails because of SNI and:

1. ansible/ansible#32053 (not backported to
   2.3.2.0)
2. ansible 1.9.6 doesn't seem to support SNI on Trusty (python < 2.7.9)
@mattt416 mattt416 force-pushed the mattt/remove_tigkstack branch from 1532918 to ba12bd5 Compare December 12, 2017 20:15
@mattt416 mattt416 changed the title DO NOT MERGE Update Add influxdata apt-keys tasks Update maas_influxdata_key variable Dec 12, 2017
@cloudnull cloudnull merged commit 8ef6a97 into master Dec 13, 2017
@odyssey4me odyssey4me deleted the mattt/remove_tigkstack branch December 13, 2017 16:18
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.

6 participants