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

Drop U16 and add U20 support #298

Merged
merged 29 commits into from
Jul 2, 2021
Merged

Conversation

winem
Copy link
Contributor

@winem winem commented Jun 8, 2021

This PR drops the support for Ubuntu 16 (xenial) and adds support for Ubuntu 20 (focal).

In addition this PR also adds meta/main.yml files for the redis and smoketests role which weren't there yet. This is rather a matter of consistency than of functionality but it seems a good chance to get this done.

It also removes support for MongoDB-Versions that are already EoL as there are 3.2 and 3.4.

Tests with st2repo_name = unstable were successful.

@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Jun 8, 2021
@arm4b arm4b requested review from cognifloyd and amanda11 June 8, 2021 17:31
@arm4b
Copy link
Member

arm4b commented Jun 8, 2021

Looks like we'll also need to add U20 to CI, per https://travis-ci.org/github/StackStorm/ansible-st2/builds/773942784 list

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

I have few minor changes and questions.

roles/StackStorm.redis/meta/main.yml Outdated Show resolved Hide resolved
roles/StackStorm.st2/tasks/auth.yml Outdated Show resolved Hide resolved
roles/StackStorm.mongodb/vars/debian_20.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@amanda11 amanda11 left a comment

Choose a reason for hiding this comment

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

As well as the comment on mongo 4.2, I think we also need updates on:

  • .kitchen.yml - has ubuntu 16 still
  • .travis.yml

roles/StackStorm.mongodb/vars/debian.yml Outdated Show resolved Hide resolved
@winem
Copy link
Contributor Author

winem commented Jun 8, 2021

Will update the ci files (travis, kitchen, etc.) till tomorrow evening.

@cognifloyd
Copy link
Member

mongodb_version needs to change for debian 20. You added an extra vars file and tasks to load that. I think it would be cleaner to consolidate the version choice in the defaults file, which looks like:

# MongoDB default version
mongodb_version: "4.0"

But you could do this:

 # MongoDB default version
-mongodb_version: "4.0" 
+mongodb_version: "{% if ansible_facts.os_family == 'Debian' and ansible_facts.distribution_major_version == '20' %}4.4{% else %}4.0{% endif %}" 

@winem
Copy link
Contributor Author

winem commented Jun 9, 2021

mongodb_version needs to change for debian 20. You added an extra vars file and tasks to load that. I think it would be cleaner to consolidate the version choice in the defaults file, which looks like:

# MongoDB default version
mongodb_version: "4.0"

But you could do this:

 # MongoDB default version
-mongodb_version: "4.0" 
+mongodb_version: "{% if ansible_facts.os_family == 'Debian' and ansible_facts.distribution_major_version == '20' %}4.4{% else %}4.0{% endif %}" 

I did it like that now. Guess that's the smartest solution to the issue and does not break with any common pattern or best practices.

Copy link
Contributor

@amanda11 amanda11 left a comment

Choose a reason for hiding this comment

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

LGTM - but wait for those others that gave review comments to give their approval.

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

LGTM

.kitchen.yml Outdated Show resolved Hide resolved
Use focal instead of xenial

Co-authored-by: Eugen Cusmaunsa <github@armab.io>
- DISTRO=ubuntu-18
- DISTRO=ubuntu-20
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to remove ubuntu-20 from here. We can't run it as a stable run yet until 3.5 is released.
So we need to do the changes for ansible-st2 in 2 stages.

  1. add it to unstable before release - under this PR
  2. add it to stable after release - separate PR

Copy link
Member

Choose a reason for hiding this comment

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

I think we'll just have to wait for a release to happen before merging this PR.

Otherwise, someone using the fresh Ansible installer will have the same issues with U16/U20.

@@ -1,3 +1,3 @@
---
# MongoDB default version
mongodb_version: "4.0"
mongodb_version: "{% if ansible_facts.os_family == 'Debian' and ansible_facts.distribution_major_version == '20' %}4.4{% else %}4.0{% endif %}"
Copy link
Member

Choose a reason for hiding this comment

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

To decide between 4.0 and 4.4 mongo version, wouldn't it be a better idea to use st2 version > 3.4 rather than only on Ubuntu 20?

Copy link
Contributor

@amanda11 amanda11 Jun 23, 2021

Choose a reason for hiding this comment

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

@rush-skills Stackstorm 3.5 still uses mongo 4.0 for all OSes EXCEPT for Ubuntu 20

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

👍
Following the #Ansible community messages, we want to merge it ASAP, considering the 3.5.0 release is ready.

Just migrated repo to the new TravisCI.com to get the build finished.

@arm4b
Copy link
Member

arm4b commented Jul 2, 2021

All green, great work everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L PR that changes 100-499 lines. Requires some effort to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants