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

MiqAeNamespace should use ancestry instead of acts_as_tree #455

Merged
merged 1 commit into from
Apr 2, 2020
Merged

MiqAeNamespace should use ancestry instead of acts_as_tree #455

merged 1 commit into from
Apr 2, 2020

Conversation

lfu
Copy link
Member

@lfu lfu commented Feb 10, 2020 •
edited
Loading

This was referenced Feb 10, 2020
Merged
Merged
Closed
@lfu lfu force-pushed the add_ancestry_to_miq_ae_namespace branch from 2dc78b3 to cb1ade8 Compare February 10, 2020 21:11
@Fryguy Fryguy self-assigned this Feb 10, 2020
Fryguy
Fryguy approved these changes Feb 10, 2020
View reviewed changes
Copy link
Member

@Fryguy Fryguy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Needs to be merged at the same time as the model changes in ManageIQ/manageiq-automation_engine#409

@lfu lfu force-pushed the add_ancestry_to_miq_ae_namespace branch from cb1ade8 to 938b057 Compare February 10, 2020 22:07
jrafanie
jrafanie reviewed Feb 18, 2020
View reviewed changes
db/migrate/20200203200855_add_ancestry_to_miq_ae_namespace.rb Outdated
@@ -0,0 +1,31 @@
require 'ancestry'

class AddAncestryToMiqAeNamespace < ActiveRecord::Migration[5.0]
Copy link
Member

Choose a reason for hiding this comment

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

If this is for master, we should be generating the migration using rails 5.1. I think the 5.0 here would still work but going forward, we'll be doing 5.1, hopefully 5.2 soon.

jrafanie
jrafanie reviewed Feb 18, 2020
View reviewed changes
manageiq-schema.gemspec
@@ -15,6 +15,7 @@ Gem::Specification.new do |s|

s.files = Dir["{db,lib}/**/*", "LICENSE.txt", "Rakefile", "README.md"]

s.add_dependency "ancestry"
Copy link
Member

Choose a reason for hiding this comment

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

@Fryguy what do you think of this dependency in schema? I believe requiring ancestry in schema migration was a thing that was possibly too much work to try to avoid.

Copy link
Member

Choose a reason for hiding this comment

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

ancestry buys us build_ancestry_from_parent_id

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with this being in schema...if it changes later, we'll have to deal with it.

otherwise we have to copy the guts of build_ancestry_from_parent_id

@lfu lfu force-pushed the add_ancestry_to_miq_ae_namespace branch from 938b057 to b50bd52 Compare February 19, 2020 16:22
@lfu lfu force-pushed the add_ancestry_to_miq_ae_namespace branch from b50bd52 to 8caf04a Compare February 24, 2020 14:46
@miq-bot
Copy link
Member

miq-bot commented Feb 24, 2020

Checked commit lfu@8caf04a with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.20.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@Fryguy Fryguy mentioned this pull request Mar 30, 2020
Closed
4 tasks
kbrock
kbrock approved these changes Mar 31, 2020
View reviewed changes
Copy link
Member

@kbrock kbrock 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

@bdunne bdunne merged commit 1cab5a1 into ManageIQ:master Apr 2, 2020
simaishi pushed a commit that referenced this pull request Apr 16, 2020
@bdunne @simaishi
Merge pull request #455 from lfu/add_ancestry_to_miq_ae_namespace
7051f7c 
MiqAeNamespace should use ancestry instead of acts_as_tree

(cherry picked from commit 1cab5a1)
@simaishi
Copy link
Contributor

Jansa backport details:

$ git log -1
commit 7051f7cbfd163947468a6e2238125f4c54784c2e
Author: Brandon Dunne <[email protected]>
Date:   Thu Apr 2 13:29:23 2020 -0400

    Merge pull request #455 from lfu/add_ancestry_to_miq_ae_namespace

    MiqAeNamespace should use ancestry instead of acts_as_tree

    (cherry picked from commit 1cab5a114a1b8ae1bbeb0231a498383c95f7bedf)

@ahrechushkin ahrechushkin mentioned this pull request Aug 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy approved these changes

@jrafanie jrafanie jrafanie left review comments

@kbrock kbrock kbrock approved these changes

@bdunne bdunne bdunne approved these changes

@tinaafitz tinaafitz tinaafitz approved these changes

Assignees

@Fryguy Fryguy

Labels
jansa/backported performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@lfu @miq-bot @simaishi @kbrock @jrafanie @Fryguy @tinaafitz @bdunne @chessbyte