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

Merge container_definition and container #15393

Merged
merged 1 commit into from
Jul 26, 2017
Merged

Merge container_definition and container #15393

merged 1 commit into from
Jul 26, 2017

Conversation

zeari
Copy link

@zeari zeari commented Jun 18, 2017 •
edited
Loading

Depends on ManageIQ/manageiq-schema#24
Merge Container and ContainerDefinition

@enoodle Please review
cc @simon3z @cben @moolitayer

@zeari
Copy link
Author

zeari commented Jun 18, 2017

@miq-bot add_label providers/containers

enoodle
enoodle reviewed Jun 18, 2017
View reviewed changes
Copy link

@enoodle enoodle left a comment

Choose a reason for hiding this comment

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

Some files should be updated in app/models/* . For example app/models/container_definitions.rb should be deleted, and app/models/container.rb should be updated.

db/migrate/20170529142557_unify_container_definition_and_container.rb Outdated
end

remove_column :containers, :container_definition_id
drop_table :container_definitions # need to take care of :dependent => destroy relats? (like ccustom attributes)
Copy link

Choose a reason for hiding this comment

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

I dont think they have custom_attributes but they have container_env_vars and container_port_configs that needs to be ported too somehow.

@zeari
Copy link
Author

zeari commented Jun 18, 2017

@miq-bot add_label wip

@miq-bot miq-bot changed the title migrations for merging container_definition and container [WIP] migrations for merging container_definition and container Jun 18, 2017
@miq-bot miq-bot added the wip label Jun 18, 2017
@zeari zeari force-pushed the unify_container_definition branch 6 times, most recently from 2a3b662 to 73eec21 Compare June 19, 2017 12:26
This was referenced Jun 19, 2017
Merged
Closed
@zeari zeari changed the title [WIP] migrations for merging container_definition and container [WIP] Merge container_definition and container Jun 19, 2017
@zeari
Copy link
Author

zeari commented Jun 19, 2017

@miq-bot remove_label wip

@miq-bot miq-bot changed the title [WIP] Merge container_definition and container Merge container_definition and container Jun 19, 2017
@miq-bot miq-bot removed the wip label Jun 19, 2017
@simon3z
Copy link
Contributor

simon3z commented Jun 21, 2017

@cben @agrare can you take a look/review? This is has implications on your graph refresh work.

@agrare
Copy link
Member

agrare commented Jun 21, 2017

@simon3z I'm good with it re: impact on graph refresh, looks like just a few of the collections will need to change.

@zeari zeari force-pushed the unify_container_definition branch from 73eec21 to 14ec0e4 Compare June 22, 2017 11:33
@simon3z
Copy link
Contributor

simon3z commented Jun 22, 2017

OK. Waiting on the UI side (still WIP).
Do we need to modify any report? cc @nimrodshn

@miq-bot
Copy link
Member

miq-bot commented Jun 22, 2017

This pull request is not mergeable. Please rebase and repush.

@Fryguy
Copy link
Member

Fryguy commented Jun 22, 2017

Database migrations have now been moved to the https://github.com/ManageIQ/manageiq-schema repo. Please see http://talk.manageiq.org/t/new-split-repo-manageiq-schema/2478 for instructions on how to transfer your database migrations. If this PR contains only migrations, I will leave it open for a short time during the transition, after which I will close this if it has not been moved over.

@zeari zeari mentioned this pull request Jun 25, 2017
Merged
@zeari zeari force-pushed the unify_container_definition branch 2 times, most recently from e90eb33 to 03fb49a Compare June 25, 2017 09:23
@miq-bot
Copy link
Member

miq-bot commented Jul 5, 2017

This pull request is not mergeable. Please rebase and repush.

@zeari zeari force-pushed the unify_container_definition branch from 7b531bd to 65ed243 Compare July 10, 2017 14:17
@miq-bot
Copy link
Member

miq-bot commented Jul 11, 2017

This pull request is not mergeable. Please rebase and repush.

@zeari zeari force-pushed the unify_container_definition branch from 65ed243 to 2ce0ffb Compare July 13, 2017 14:56
@zeari
Copy link
Author

zeari commented Jul 13, 2017

@Fryguy @blomquisg can you have a look here? this PR is tightly coupled with ManageIQ/manageiq-schema#24 and both should be ready to merge

@moolitayer
Copy link

@simon3z @cben @enoodle can you please approve this PR?

@Fryguy Fryguy closed this Jul 17, 2017
@Fryguy Fryguy reopened this Jul 17, 2017
@zeari zeari force-pushed the unify_container_definition branch 2 times, most recently from 6d4ccae to 6a8eb8a Compare July 20, 2017 14:01
@zeari zeari force-pushed the unify_container_definition branch from 6a8eb8a to 750d3c4 Compare July 20, 2017 14:14
@miq-bot
Copy link
Member

miq-bot commented Jul 20, 2017

Checked commit zeari@750d3c4 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
8 files checked, 1 offense detected

app/models/container_group.rb

enoodle
enoodle approved these changes Jul 24, 2017
View reviewed changes
Copy link

@enoodle enoodle left a comment

Choose a reason for hiding this comment

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

LGTM

@moolitayer
Copy link

cc @Loicavenel

@Fryguy Fryguy closed this Jul 25, 2017
@Fryguy Fryguy reopened this Jul 25, 2017
@Fryguy
Copy link
Member

Fryguy commented Jul 25, 2017

Merged the schema PR, so just rekicking to runs the specs.

@Fryguy Fryguy merged commit b4554f7 into ManageIQ:master Jul 26, 2017
@Fryguy Fryguy added this to the Sprint 66 Ending Aug 7, 2017 milestone Jul 26, 2017
@himdel himdel mentioned this pull request Jul 26, 2017
Merged
@himdel
Copy link
Contributor

himdel commented Jul 26, 2017

Just a rant here, but it would be nice if definitely-breaking changes such as these were actually tested in the UI .. just opening a container summary screen blows up ;). (Fixed by ManageIQ/manageiq-ui-classic#1760, I hope.)

@zeari
Copy link
Author

zeari commented Jul 26, 2017

Just a rant here, but it would be nice if definitely-breaking changes such as these were actually tested in the UI .. just opening a container summary screen blows up ;). (Fixed by ManageIQ/manageiq-ui-classic#1760, I hope.)

@himdel I feel your pain :( but --> #15393 (comment)
Ill link to the UI PR in the description next time

@gmcculloug gmcculloug mentioned this pull request Jul 26, 2017
Merged
@zeari zeari mentioned this pull request Jul 27, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simon3z simon3z simon3z left review comments

@enoodle enoodle enoodle approved these changes

@cben cben cben approved these changes

Assignees

@simon3z simon3z

Labels
providers/containers sql migration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@zeari @simon3z @agrare @miq-bot @Fryguy @nimrodshn @moolitayer @himdel @cben @enoodle