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.

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

Introduce mutli platform .travis #31

Conversation

ernestas-poskus
Copy link
Member

@ernestas-poskus ernestas-poskus commented Sep 9, 2016

Replaced init check with ansible_service_mgr fact.
Added systemctl daemon-reload.
Added /etc/sysconfig creation (Ubuntu 16.04) does no have this.
Bumped marathon to 1.3.6.
Introduced tests on 4 platforms of two Linux distributions.

@ernestas-poskus ernestas-poskus force-pushed the features/introduce_multi_platform_travis branch 30 times, most recently from 77cf9e6 to 1a39a15 Compare September 10, 2016 15:27
@ernestas-poskus ernestas-poskus force-pushed the features/introduce_multi_platform_travis branch 3 times, most recently from da5a761 to ab2a28b Compare December 1, 2016 08:52
@ernestas-poskus ernestas-poskus force-pushed the features/introduce_multi_platform_travis branch from ab2a28b to b738166 Compare December 1, 2016 09:05
@ernestas-poskus
Copy link
Member Author

@veger @lhoss please review, the plan is to merge this first and then check on #39.

With next PR will update README and release under 0.6.0

@@ -1,9 +1,9 @@
---
marathon_playbook_version: "0.3.4"
marathon_version: "0.8.1"
marathon_version: "1.3.6"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍
using v1.3.6 (was latest 1 week ago) in prod for almost 1 week. Looks good!

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

notify: Restart marathon

- name: Create /etc/sysconfig directory
file: path=/etc/sysconfig state=directory owner=root group=root mode=0755
Copy link
Contributor

Choose a reason for hiding this comment

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

That sysconfig folder seems to be outdated (was only used in redhat-style OS, says ->link), then I'm wondering if those env-vars (by default only JAVA_OPTS) are used at all on Ubuntu systemd.
The new recommended way (both redhat/ubuntu) seems to be in /etc/systemd/system/<service-name>.service.d/<something>.conf
Resf:

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right I doubt they are used on Ubuntu maybe even on RedHat as well. In my current company I was recently migrating init to systemd and was setuping service.d folders as well so this might be good option to implement it. Systemd supports EnvironmentFile as well but since we use prepacked service it might not be an option here.

- role: geerlingguy.java
java_packages:
- java-1.8.0-openjdk
when: ansible_os_family == 'RedHat'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'ld expect a solution involving less ansible boilerplate, to simply install a current (open)jdk-8 for different distros (here debian/ubuntu & redhat). or really not?

Copy link
Member Author

Choose a reason for hiding this comment

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

My thought was to update README by introducing such dependency to install java 8 because I don't want this playbook to be jack of all trades.

Copy link
Contributor

Choose a reason for hiding this comment

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

fully agree 👍
Already >1 year ago I actually proposed the same (removing the java-dep) from another public role (ansible-spark), that we improved+use ourselves
and thought about this beeing a general problem in ansible dependency mgmt, resulting in ansible/ansible#13215 ...

@@ -1,3 +1,7 @@
---
- name: Restart marathon
service: name=marathon state=restarted

- name: Reload daemon
command: systemctl daemon-reload
Copy link
Contributor

@veger veger Dec 2, 2016

Choose a reason for hiding this comment

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

Use systemd module: https://docs.ansible.com/ansible/systemd_module.html
Or do we need to support Ansible version <2.2?

Copy link
Contributor

@lhoss lhoss Dec 2, 2016

Choose a reason for hiding this comment

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

tja, If we could have a statistic about the current ansible-versions used (by users of this role)!
IMO it's a bit early to require 2.2.x, but requiring >2(.1).x OK for me.
To be honest in our company <30% upgraded to 2.2.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to require higher Ansible version for this change alone, let's give it some time.

@ernestas-poskus ernestas-poskus force-pushed the features/introduce_multi_platform_travis branch 7 times, most recently from 9132559 to b50e6ff Compare December 8, 2016 08:06
Increase level of verbosity when install test role
@ernestas-poskus ernestas-poskus force-pushed the features/introduce_multi_platform_travis branch 6 times, most recently from 744cf96 to ee7df6e Compare December 8, 2016 09:18
@ernestas-poskus ernestas-poskus force-pushed the features/introduce_multi_platform_travis branch from ee7df6e to 4cd047b Compare December 8, 2016 09:54
@ernestas-poskus
Copy link
Member Author

Made environment variables load under sytemd .d directory validated with tests.

@ernestas-poskus ernestas-poskus merged commit 6bb0404 into AnsibleShipyard:master Dec 8, 2016
@ernestas-poskus ernestas-poskus deleted the features/introduce_multi_platform_travis branch December 8, 2016 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants