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

Pinning faker gem to 1.8.3 #118

Merged
merged 1 commit into from
Dec 7, 2017
Merged

Pinning faker gem to 1.8.3 #118

merged 1 commit into from
Dec 7, 2017

Conversation

juliancheal
Copy link
Member

@juliancheal juliancheal commented Dec 7, 2017

faker changed dependency of i18n which conflicts with our dependency from manageiq-consumption:
faker-ruby/faker@9128945#diff-e79a60dc6b85309ae70a6ea8261eaf95R5

@juliancheal
Copy link
Member Author

@jrafanie This ok?

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

Looks good, can you include why we're doing it?

faker changed dependency of i18n which conflicts with our dependency from manageiq-consumption:
faker-ruby/faker@9128945#diff-e79a60dc6b85309ae70a6ea8261eaf95R5

@juliancheal
Copy link
Member Author

Looks good, can you include why we're doing it?

@jrafanie Because you said to? 😂

faker changed dependency of i18n which conflicts with our dependency from manageiq-consumption:
faker-ruby/faker@9128945#diff-e79a60dc6b85309ae70a6ea8261eaf95R5
@miq-bot
Copy link
Member

miq-bot commented Dec 7, 2017

Checked commit e901c89 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

@juliancheal
Copy link
Member Author

@miq-bot assign @agrare

@agrare
Copy link
Member

agrare commented Dec 7, 2017

Looks good, can you include why we're doing it?

@jrafanie Because you said to? 😂

Loool

@agrare agrare merged commit 7d0ed91 into ManageIQ:master Dec 7, 2017
@agrare agrare added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 7, 2017
Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

:shipit:

@jrafanie
Copy link
Member

jrafanie commented Dec 7, 2017

Thanks @juliancheal

@juliancheal juliancheal deleted the pin_faker_gem branch December 7, 2017 16:43
@simaishi
Copy link
Contributor

simaishi commented Dec 8, 2017

Adding gaprindashvili/yes, as this affects gaprindashvili branch as well.

In the last build, 1.8.4 was being used. Do we not want to pin at 1.8.4?

@juliancheal
Copy link
Member Author

@simaishi oh that's strange as it's always been 1.8.3 in the gemspec. @jrafanie would 1.8.4 cause you the issues you were seeing?

@simaishi
Copy link
Contributor

simaishi commented Dec 8, 2017

The gemspec had ~>1.8.3, which was resolving to 1.8.4 until 1.8.5 came out a few days ago, which caused the problem.

1.8.4 requires i18n ~> 0.5

@jrafanie
Copy link
Member

jrafanie commented Dec 8, 2017

1.8.5 is the problem version so 1.8.4 should be fine to use @juliancheal

@juliancheal
Copy link
Member Author

@jrafanie @simaishi cool beans. Ok 1.8.4 it is. Thanks @simaishi :)

simaishi pushed a commit that referenced this pull request Dec 8, 2017
Pinning faker gem to 1.8.3
(cherry picked from commit 7d0ed91)
@simaishi
Copy link
Contributor

simaishi commented Dec 8, 2017

Gaprindashvili backport details:

$ git log -1
commit 108d4801ef8edd0760f33b01951f17a67a05d7a0
Author: Adam Grare <[email protected]>
Date:   Thu Dec 7 10:59:55 2017 -0500

    Merge pull request #118 from juliancheal/pin_faker_gem
    
    Pinning faker gem to 1.8.3
    (cherry picked from commit 7d0ed913905806800584dc6d3a8526310eea597d)

@chargio
Copy link

chargio commented Dec 10, 2017

We can update the dependency on manageiq-consumption, if it is better.

@jrafanie
Copy link
Member

@chargio It's not mutually exclusive. This change requires no other changes. We can now upgrade manageiq-consumption at our convenience and then decide if we want to loosen the dependency requirement here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants