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

Adding present option for version #148

Merged
merged 5 commits into from
Jun 8, 2017
Merged

Adding present option for version #148

merged 5 commits into from
Jun 8, 2017

Conversation

bigmstone
Copy link
Contributor

@bigmstone bigmstone commented May 26, 2017

Name says it all.

Fixes #111

@m4dcoder
Copy link
Contributor

LGTM but i should let @humblearner or @armab approve since they're more familiar with this.

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.

Instead of introducing another keyword present, I'd like us to use existing state: present block and improve existing logic. See also the following issues for more context to understand the history:
#142 and #111

Ideally is to have the following syntax working:

  • st2_version: latest - as before, always tries to auto-update the package.
  • st2_version: * - this is exactly what's you're requesting, just make * working instead of present naming. (see UPDATE in the next message)
  • st2_version: 2.2.1 - pin st2 version, but use latest st2_revision.
  • st2_version: 2.2.1, st2_revision: 5 - pin both package version and revision.

- restart st2
- reload st2
tags: st2, skip_ansible_lint

- name: Install pinned st2 package
become: yes
package:
name: st2={{ st2_version }}-{{ st2_revision }}
state: present
Copy link
Member

@arm4b arm4b May 29, 2017

Choose a reason for hiding this comment

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

We already have state: present logic here.

Thinking about how to use that block, instead of introducing another st2_version: present keyword.

@arm4b
Copy link
Member

arm4b commented May 29, 2017

After playing a bit with st2_version: * st2_version: 2.3dev + st2_revision: * it looks like * works for auto-updating, despite Ansible explicit state: present flag.

Le me think a little & dig in further, including how Ansible package module works to make sure the syntax we'll use will be consistent enough across the use cases for st2_version & st2_revision.

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.

UPDATE:

So after further research and things like https://github.com/ansible/ansible/blob/6dd8a4cf78d4fe4216a842a461871bade9a9b5a2/lib/ansible/modules/packaging/os/apt.py#L496-L501 and https://github.com/ansible/ansible/blob/6dd8a4cf78d4fe4216a842a461871bade9a9b5a2/lib/ansible/modules/packaging/os/apt.py#L1000, here is an updated alternative syntax which I believe will work better, comparing to introducing new present keyword:

  • st2_version: null = just install st2 package (equivalent: apt/yum install st2) (no upgrade, NEW default). Exactly what you're requesting.
  • st2_version: latest = always auto-upgrade to use latest package (OLD default)
  • st2_version: 2.3 = pinned st2 package (equivalent: apt/yum install st2=2.3-*)

@bigmstone @humblearner @lakshmi-kannan WDYT?

@bigmstone
Copy link
Contributor Author

bigmstone commented May 31, 2017

@armab thoughts?
https://github.com/StackStorm/ansible-st2/pull/148/files#diff-555e17548189fd960a2fbb76681e59d9R13

If you like the direction I'll make the rest match.

@arm4b
Copy link
Member

arm4b commented May 31, 2017

@bigmstone We've talked about the st2_version: null as var value, not st2_version: present.

So with st2_version: null we do apt/yum install st2.

become: yes
package:
name: st2
state: latest
when: st2_version == "latest"
Copy link
Member

Choose a reason for hiding this comment

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

I'm more about leaving latest block as is for less jinja hackery.

@arm4b
Copy link
Member

arm4b commented Jun 5, 2017

Looks there is no easy fix for #111

For example this construction will pin the version, but always try to update the package once there is a new revision available in a repo. So no real pinning happens here:

st2_version: 2.2.1
st2_revision: *

Another idea is that we could check if package is installed for apt/yum separately, parse the version, extract the revision and compare with the user input st2_version & st2_revision. But this logic easily breaks by the fact that user is free to specify anything like st2_version: 2.2.*. Also, it's probably a bad show http://blog.jasonantman.com/2014/07/how-yum-and-rpm-compare-versions/ without wrapping the logic as a custom Ansible plugin with some Python.

@arm4b arm4b force-pushed the fix/present branch 3 times, most recently from 5bc1fa8 to 632b6ca Compare June 7, 2017 18:16
- restart st2
- reload st2
tags: st2

- name: Install pinned st2 package
become: yes
package:
name: st2={{ st2_version }}-{{ st2_revision }}
Copy link
Member

@arm4b arm4b Jun 7, 2017

Choose a reason for hiding this comment

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

After check, needs an adjustement as well, since it's apt format: apt install st2=2.2.1-5.
yum is different: yum install st2-2.2.1-5.

It's interesting that revision is not a requirement for yum. It's possible to do: yum install st2-2.2.1, while it's not possible to specify version without revision in apt: apt install st2=2.2.1


In short: this block never worked in CentOS/EL. Something that we missed in a big v0.6.0

@arm4b arm4b self-assigned this Jun 7, 2017
@arm4b arm4b added the WIP label Jun 7, 2017
@arm4b
Copy link
Member

arm4b commented Jun 7, 2017

@bigmstone FYI I reverted back to your initial commit with present.
Sorry for the cofusion, but I initially wanted us to improve the versioning story here, which is already confusing and requires a good overhaul considering corner cases with apt/yum and package versioning.

Will see/experiment what I can do in this and next PRs.

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.

Alright, merging initial Matt's present proposal.

Fixed also one apt/yum-related corner case. But as said before, initially thought abount fixing more here, but moving entire version overhaul into another issue: #150 which will probably be the breaking change.

@arm4b arm4b merged commit ad08d66 into master Jun 8, 2017
@arm4b arm4b deleted the fix/present branch June 8, 2017 15:14
@arm4b arm4b added the feature label Jun 8, 2017
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.

None yet

3 participants