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

Added a new matcher for amazon linux 2 #380

Merged
merged 1 commit into from
Nov 27, 2018
Merged

Added a new matcher for amazon linux 2 #380

merged 1 commit into from
Nov 27, 2018

Conversation

artyomtkachenko
Copy link
Contributor

Th target of this PR is to address a problem which appeared after AWS updated naming schema for Amazon Linux 2.

More details can be found here

@fatbasstard
Copy link

Running into this issue. A fix being released would be really nice 👍

@chris-rock
Copy link
Contributor

@artyomtkachenko Can you please sign your commit?

@chris-rock
Copy link
Contributor

chris-rock commented Nov 21, 2018

Can you also share where and how you experienced it? While I found references on the net, I could not reproduce the issue with latest amazon 2 docker image.

Related to ansible/ansible#48823

Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

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

Thank you for your first-time contribution @artyomtkachenko

@radhus
Copy link

radhus commented Nov 21, 2018

I experience this and can confirm that this fix solves the problem. I just run an EC2 instance with the latest Amazon Linux 2 AMI, fully updated, then run the latest inspec with some tests, and this started crashing since a week or so.

@radhus
Copy link

radhus commented Nov 22, 2018

@chris-rock you can reproduce it with the latest amazonlinux:2 docker image if you run yum update -y prior to inspec

@chris-rock
Copy link
Contributor

@artyomtkachenko Is there any way we can help you? We would love to get your PR in

@jerryaldrichiii
Copy link
Contributor

Just realized I solved this in #385 ...sorry I didn't see this PR.

@artyomtkachenko in order to get you credit for this (which I really want to do 😄) you can do the following:

  • Update this PR with the RegEx from https://github.com/inspec/train/pull/385/files (yours works due to the else, but I think mine is a bit cleaner)
  • Do a git commit amend -s and git push origin master -f (DCO sign off) (I'll pull your commit over and into mine)
  • Leave a comment here that you don't mind being in the Git history (I'll still give you a shout out in the message/title field)

@artyomtkachenko
Copy link
Contributor Author

Please accept my apologies Guys for a delay in a reply (all Github messages went into a SPAM folder for some reason ) and for a missing DCO. I updated my PR with the cleaner solution from @jerryaldrichiii .
Thank you very much again
👍

@jerryaldrichiii
Copy link
Contributor

jerryaldrichiii commented Nov 27, 2018

@artyomtkachenko the RegEx is still a bit off. AMI is not always present.

Can you change to have that method look like:

    def redhatish_version(conf)
      case conf
      when /rawhide/i
        /((\d+) \(Rawhide\))/i.match(conf)[1].downcase
      when /Amazon Linux/i
        /([\d\.]+)/.match(conf)[1]
      when /derived from .*linux|amazon/i
        /Linux ((\d+|\.)+)/i.match(conf)[1]
      else
        /release ([\d\.]+)/.match(conf)[1]
      end
    end

EDIT: This is probably my fault. I'm am being too broad when I say "the RegEx". What I really mean is both the RegEx for the case statement (/Amazon Linux/i) and the matching part under it (/([\d\.]+)/.match(conf)[1]). This change will allow both types of the Amazon Linux string to be caught in the same area instead of relying on multiple when statements or the else statement.

Signed-off-by: Artyom Tkachenko <offtema@gmail.com>
@artyomtkachenko
Copy link
Contributor Author

@artyomtkachenko the RegEx is still a bit off. AMI is not always present.

Can you change to have that method look like:

    def redhatish_version(conf)
      case conf
      when /rawhide/i
        /((\d+) \(Rawhide\))/i.match(conf)[1].downcase
      when /Amazon Linux/i
        /([\d\.]+)/.match(conf)[1]
      when /derived from .*linux|amazon/i
        /Linux ((\d+|\.)+)/i.match(conf)[1]
      else
        /release ([\d\.]+)/.match(conf)[1]
      end
    end

Sorry, my mstake. Forgot to run git add ....

Copy link
Contributor

@jerryaldrichiii jerryaldrichiii left a comment

Choose a reason for hiding this comment

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

Great work @artyomtkachenko!

@jquick jquick merged commit 698621a into inspec:master Nov 27, 2018
@apogrebnyak
Copy link

Could you also add an exception handler ?

def redhatish_version(conf)
  begin
      case conf
      when /rawhide/i
        /((\d+) \(Rawhide\))/i.match(conf)[1].downcase
      when /Amazon Linux/i
        /([\d\.]+)/.match(conf)[1]
      when /derived from .*linux|amazon/i
        /Linux ((\d+|\.)+)/i.match(conf)[1]
      else
        /release ([\d\.]+)/.match(conf)[1]
      end
  rescue => ex
    raise RuntimeError, "Cannot parse redhatish OS version", ex.backtrace
  end
end

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.

7 participants