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.

Sign up for GitHub

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

Jump to bottom

Create TransformationMapping before Item (FIX CI FAILURE) #666

Merged
merged 1 commit into from
Aug 29, 2019
Merged

Create TransformationMapping before Item (FIX CI FAILURE) #666

merged 1 commit into from
Aug 29, 2019

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Aug 29, 2019 •
edited
Loading

FIXing CI failure https://travis-ci.org/ManageIQ/manageiq-api/jobs/578155651

After this ManageIQ/manageiq#19204 it looks like that when we are creating

TransformationMapping with TransformationMappingItems( with DataStorage which requires created TransformationMapping )

we need to create first
TransformationMapping

and then create TransformationMappingItems and then assign TransformationMapping

It looks like that there is also issue in create operation https://github.com/ManageIQ/manageiq-api/blob/ba3258bef9b483b3c0cf5d9c950f62d235bbba91/app/controllers/api/transformation_mappings_controller.rb

please review @agrare @thearifsmail @djberg96

@miq-bot assign @abellotti

@lpichler lpichler added the bug label Aug 29, 2019
@lpichler lpichler requested a review from djberg96 August 29, 2019 09:58
@lpichler lpichler requested a review from agrare August 29, 2019 09:58
@lpichler lpichler force-pushed the use_factory_bot_for_transformation_mapping_item branch from ba3258b to c72459a Compare August 29, 2019 11:54
@agrare
Copy link
Member

agrare commented Aug 29, 2019

@lpichler this looks like a dup of #664

@lpichler
Copy link
Contributor Author

@agrare yes, expect to specs which are needed for fix CI failure.

@djberg96
CI is broken because of

@djberg96 so can you take my changes about FactoryBot.create in order to fix CI in one PR ?

thanks

djberg96
djberg96 reviewed Aug 29, 2019
View reviewed changes
app/controllers/api/transformation_mappings_controller.rb Outdated
transformation_mapping.transformation_mapping_items = create_mapping_items(data["transformation_mapping_items"])
transformation_mapping.save!
transformation_mapping_items = create_mapping_items(data["transformation_mapping_items"])
transformation_mapping_items.each { |tmi| tmi.update(:transformation_mapping => transformation_mapping) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just call .save! here?

Copy link
Member

Choose a reason for hiding this comment

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

I liked what @djberg96 had where he passed the mapping to the TransformationMappingItem.new, then yeah just .save! at the end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used it 👍 (of course with @djberg96 approval :) )

@agrare
Copy link
Member

agrare commented Aug 29, 2019

@lpichler I think you need to do the same thing for create_resource

@lpichler lpichler force-pushed the use_factory_bot_for_transformation_mapping_item branch from c72459a to 2606908 Compare August 29, 2019 16:32
agrare
agrare approved these changes Aug 29, 2019
View reviewed changes
Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

👍 looks great thanks @djberg96 and @lpichler

@lpichler
Copy link
Contributor Author

thanks @agrare @djberg96 👍

@miq-bot
Copy link
Member

miq-bot commented Aug 29, 2019

Checked commit lpichler@2606908 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@abellotti abellotti merged commit 5675728 into ManageIQ:master Aug 29, 2019
@abellotti abellotti added this to the Sprint 119 Ending Sep 2, 2019 milestone Aug 29, 2019
@djberg96
Copy link
Contributor

The PR of the beast!

@djberg96 djberg96 mentioned this pull request Aug 29, 2019
Closed
@lpichler lpichler deleted the use_factory_bot_for_transformation_mapping_item branch August 29, 2019 16:58
@simaishi
Copy link
Contributor

simaishi commented Nov 4, 2019 •
edited
Loading

@lpichler @djberg96 can this be ivanchuk/yes? ManageIQ/manageiq#19204 is ivanchuk/yes

@djberg96
Copy link
Contributor

djberg96 commented Nov 4, 2019

@simaishi I think it's ok. @lpichler any objections?

@lpichler
Copy link
Contributor Author

lpichler commented Nov 4, 2019

@simaishi I think it's ok. @lpichler any objections?

No ,I don't have any.so can go to ivanchuk. Thanks @djberg96.

simaishi pushed a commit that referenced this pull request Nov 4, 2019
@abellotti @simaishi
Merge pull request #666 from lpichler/use_factory_bot_for_transformat…
b233df9 
…ion_mapping_item

Create TransformationMapping before Item (FIX CI FAILURE)

(cherry picked from commit 5675728)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1768517
@simaishi
Copy link
Contributor

simaishi commented Nov 4, 2019

Ivanchuk backport details:

$ git log -1
commit b233df92d1c1af00048280188166498489da75af
Author: Alberto Bellotti <[email protected]>
Date:   Thu Aug 29 12:55:17 2019 -0400

    Merge pull request #666 from lpichler/use_factory_bot_for_transformation_mapping_item
    
    Create TransformationMapping before Item (FIX CI FAILURE)
    
    (cherry picked from commit 567572881a3d61093341cf581bec4d0e2bd2abcb)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1768517

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agrare agrare agrare approved these changes

@abellotti abellotti abellotti approved these changes

@djberg96 djberg96 djberg96 approved these changes

Assignees

@abellotti abellotti

Labels
bug ivanchuk/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lpichler @agrare @miq-bot @djberg96 @simaishi @abellotti