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

Fixes #21424 - use factory_bot_rails #4935

Merged
merged 1 commit into from Nov 1, 2017

Conversation

Projects
None yet
7 participants
@theforeman-bot

This comment has been minimized.

Show comment
Hide comment
@theforeman-bot
Member

theforeman-bot commented Oct 23, 2017

Issues: #21424

@timogoebel

This comment has been minimized.

Show comment
Hide comment
@timogoebel

timogoebel Oct 23, 2017

Member

👎 for them changing the name of the gem.

@ares: Thanks for fixing this, but I think factory_girl 4.8.2 (that included the rename) was yanked from rubygems.org. So tests should be fine for now, I believe.

Member

timogoebel commented Oct 23, 2017

👎 for them changing the name of the gem.

@ares: Thanks for fixing this, but I think factory_girl 4.8.2 (that included the rename) was yanked from rubygems.org. So tests should be fine for now, I believe.

@tbrisker

This comment has been minimized.

Show comment
Hide comment
@tbrisker

tbrisker Oct 23, 2017

Member

@ares thanks, I think you also need to s/FactoryGirl/FactoryBot everywhere.

Member

tbrisker commented Oct 23, 2017

@ares thanks, I think you also need to s/FactoryGirl/FactoryBot everywhere.

@ares

This comment has been minimized.

Show comment
Hide comment
@ares

ares Oct 23, 2017

Member

Yeah, I'll rename the rest, we can hold on until they release a new version but I suppose we want to use newer versions of the gem regardless of its name. Let's keep the discussion about renaming it out of Foreman. If anyone wants to participate in it - thoughtbot/factory_bot#921

Member

ares commented Oct 23, 2017

Yeah, I'll rename the rest, we can hold on until they release a new version but I suppose we want to use newer versions of the gem regardless of its name. Let's keep the discussion about renaming it out of Foreman. If anyone wants to participate in it - thoughtbot/factory_bot#921

Show outdated Hide outdated test/models/orchestration/tftp_test.rb Outdated
Show outdated Hide outdated test/factories/host_related.rb Outdated
@ekohl

This comment has been minimized.

Show comment
Hide comment
@ekohl

ekohl Oct 23, 2017

Member

[test foreman]

Member

ekohl commented Oct 23, 2017

[test foreman]

@ares

This comment has been minimized.

Show comment
Hide comment
@ares

ares Oct 24, 2017

Member

it would be good to get this in soon, it's getting out of date with every PR that adds new tests with FactoryGirl

Member

ares commented Oct 24, 2017

it would be good to get this in soon, it's getting out of date with every PR that adds new tests with FactoryGirl

@timogoebel

This comment has been minimized.

Show comment
Hide comment
@timogoebel

timogoebel Oct 25, 2017

Member

[test katello]

Member

timogoebel commented Oct 25, 2017

[test katello]

@tbrisker

This comment has been minimized.

Show comment
Hide comment
@tbrisker

tbrisker Oct 25, 2017

Member

I suggest sending a warning to foreman-dev list about this asking all plugin maintainers to prepare PRs with s/FactoryGirl/FactoryBot/g, and merge a couple of days after that. I opened #4948 to unblock CI for now.

Member

tbrisker commented Oct 25, 2017

I suggest sending a warning to foreman-dev list about this asking all plugin maintainers to prepare PRs with s/FactoryGirl/FactoryBot/g, and merge a couple of days after that. I opened #4948 to unblock CI for now.

@lzap

This comment has been minimized.

Show comment
Hide comment
@lzap

lzap Oct 25, 2017

Member

How about an alias class and a ticket to deprecate it in 1.18?

Member

lzap commented Oct 25, 2017

How about an alias class and a ticket to deprecate it in 1.18?

@tbrisker

This comment has been minimized.

Show comment
Hide comment
@tbrisker

tbrisker Oct 25, 2017

Member

Sent out a mail to foreman-dev giving heads up.
@ares - Please rebase.
@theforeman/core - please DO NOT MERGE until November 1st.

Member

tbrisker commented Oct 25, 2017

Sent out a mail to foreman-dev giving heads up.
@ares - Please rebase.
@theforeman/core - please DO NOT MERGE until November 1st.

@ares

This comment has been minimized.

Show comment
Hide comment
@ares

ares Oct 25, 2017

Member

I'll rebase on Nov 1, every PR that adds a test using FG breaks this.

Member

ares commented Oct 25, 2017

I'll rebase on Nov 1, every PR that adds a test using FG breaks this.

@ares

This comment has been minimized.

Show comment
Hide comment
@ares

ares Nov 1, 2017

Member

@tbrisker I pushed the new version, I'd kindly ask to not merge other PRs before this gets in. Could you please take a look before tests are finished so we could start the merge chain ASAP? Meanwhile I'll go over plugins and will send out PR if I don't see them opened.

Member

ares commented Nov 1, 2017

@tbrisker I pushed the new version, I'd kindly ask to not merge other PRs before this gets in. Could you please take a look before tests are finished so we could start the merge chain ASAP? Meanwhile I'll go over plugins and will send out PR if I don't see them opened.

@tbrisker

This comment has been minimized.

Show comment
Hide comment
@tbrisker

tbrisker Nov 1, 2017

Member

Ack, waiting for tests to pass and then i'll merge.

Member

tbrisker commented Nov 1, 2017

Ack, waiting for tests to pass and then i'll merge.

@tbrisker

This comment has been minimized.

Show comment
Hide comment
@tbrisker

tbrisker Nov 1, 2017

Member

@ares - looks like you missed a couple:

2.4.0@foreman_gemset ~/foreman (ares-fix/21424) $ git grep -i girl
test/controllers/api/v2/domains_controller_test.rb:    domain = FactoryGirl.create(:domain)
test/controllers/api/v2/domains_controller_test.rb:    org = FactoryGirl.create(:organization)
Member

tbrisker commented Nov 1, 2017

@ares - looks like you missed a couple:

2.4.0@foreman_gemset ~/foreman (ares-fix/21424) $ git grep -i girl
test/controllers/api/v2/domains_controller_test.rb:    domain = FactoryGirl.create(:domain)
test/controllers/api/v2/domains_controller_test.rb:    org = FactoryGirl.create(:organization)
@ares

This comment has been minimized.

Show comment
Hide comment
@ares

ares Nov 1, 2017

Member

Yeah it seems I missed those on last rebase, should be fixed now

Member

ares commented Nov 1, 2017

Yeah it seems I missed those on last rebase, should be fixed now

@tbrisker

Test failure is unrelated, merging. Thanks @ares !

@tbrisker tbrisker merged commit 8c6bc83 into theforeman:develop Nov 1, 2017

5 of 7 checks passed

foreman Build finished.
Details
katello Build finished.
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
prprocessor Commit message style is correct
Details
upgrade Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment