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

Renamed foreman features to ems_configuration and adjusted roles #464

Merged
merged 1 commit into from
May 8, 2020

Conversation

skateman
Copy link
Member

@skateman skateman commented Apr 3, 2020

Adjusting the user roles to the renamed foreman features. This isn't the final form yet as the name of the features might change in the core PR.

Core PR: ManageIQ/manageiq#19949
UI PR: ManageIQ/manageiq-ui-classic#6782

@miq-bot miq-bot added the wip label Apr 3, 2020
@skateman skateman force-pushed the rename-foreman-features branch 3 times, most recently from 146a0cf to 780eaa0 Compare April 3, 2020 14:12
@skateman
Copy link
Member Author

skateman commented Apr 3, 2020

@Fryguy this is how far I could get, I am totally unsure why that one case is failing 😕

@Fryguy
Copy link
Member

Fryguy commented Apr 6, 2020

@bdunne Can you also lend some eyes to this?

@Fryguy
Copy link
Member

Fryguy commented Apr 6, 2020

The reason the specs are failing is because data migrations should leave a previously empty database empty afterwards. This is the base case for a brand new installation, where the database is completely empty and when you migrate up it should still be empty. This is important for things like the test database.

The way to ensure this is to see if there are existing records that need to be migrated before migrating anything. If there are no records, there's nothing to "fix", so you can skip the rest.

@skateman
Copy link
Member Author

skateman commented Apr 7, 2020

@Fryguy I understand what's wrong with the specs, but I don't really understand what should I do to fix them.

@skateman skateman force-pushed the rename-foreman-features branch 2 times, most recently from 6af46eb to ace9d1e Compare April 7, 2020 10:09
@Fryguy
Copy link
Member

Fryguy commented Apr 7, 2020

@skateman Oh, just add this as a first line in both the up and the down

return if MiqProductFeature.none?

@skateman skateman force-pushed the rename-foreman-features branch from ace9d1e to 2567294 Compare April 7, 2020 14:22
@skateman
Copy link
Member Author

skateman commented Apr 7, 2020

@Fryguy ok, now it makes sense what you wrote before, thanks.

@skateman skateman force-pushed the rename-foreman-features branch from 2567294 to 211df78 Compare April 7, 2020 14:52
@skateman skateman force-pushed the rename-foreman-features branch from 211df78 to b65e48e Compare April 8, 2020 08:25
@skateman skateman changed the title [WIP] Renamed foreman features to ems_comfiguration and adjusted roles Renamed foreman features to ems_comfiguration and adjusted roles Apr 8, 2020
@miq-bot miq-bot removed the wip label Apr 8, 2020
@Fryguy
Copy link
Member

Fryguy commented Apr 8, 2020

Found one last thing, otherwise LGTM

@skateman skateman force-pushed the rename-foreman-features branch from b65e48e to 8fdeba8 Compare April 8, 2020 13:20
@skateman skateman changed the title Renamed foreman features to ems_comfiguration and adjusted roles [WIP] Renamed foreman features to ems_comfiguration and adjusted roles Apr 9, 2020
@skateman
Copy link
Member Author

skateman commented Apr 9, 2020

@h-kataria found some issues, moving this back to WIP

@miq-bot miq-bot added the wip label Apr 9, 2020
@skateman skateman force-pushed the rename-foreman-features branch from 8fdeba8 to 7662d7f Compare April 14, 2020 14:47
@h-kataria
Copy link
Contributor

tested, LGTM.

@skateman skateman force-pushed the rename-foreman-features branch from f46f070 to c6ffacf Compare April 21, 2020 17:37
return if MiqProductFeature.none?

say_with_time 'Renaming new ems configuration features to old foreman explorer ones' do
FEATURE_MAPPING.each do |to, from|
Copy link
Member

Choose a reason for hiding this comment

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

There a handful of query-in-a-loop patterns here that can be really inefficient, but we're not talking a ton of identifiers here, so maybe its not really an issue, and the code is pretty elegant to read as is. I'll leave it up to you. I think the alternative would look something like...

features = MiqProductFeature.all.index_by(&:identifier)
...
FEATURE_MAPPING.each do |from, to|
  features[from]&.update!(...)
end
...
ROLES_FEATURE_MAPPING.each do |from, to|
  from_feature = (features[from] ||= MiqProductFeature.create!(...)
  to_features = to.map { |identifier| (features[to] ||= MiqProductFeature.create!(...)  }
...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to leave it as it is, for readability. We don't really have millions of features.

@Fryguy
Copy link
Member

Fryguy commented Apr 21, 2020

This PR is awesome...just a couple more minor comments.

@Fryguy
Copy link
Member

Fryguy commented Apr 21, 2020

Also seems the Travis issues are legit.

@skateman skateman force-pushed the rename-foreman-features branch from c6ffacf to 0269302 Compare April 21, 2020 17:49
@Fryguy Fryguy changed the title Renamed foreman features to ems_comfiguration and adjusted roles Renamed foreman features to ems_configuration and adjusted roles Apr 21, 2020
@skateman skateman force-pushed the rename-foreman-features branch 4 times, most recently from fd5b70f to 97518e4 Compare April 22, 2020 08:07

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@skateman skateman force-pushed the rename-foreman-features branch from 97518e4 to b4a8b33 Compare April 22, 2020 08:12
h-kataria added a commit to h-kataria/manageiq-cross_repo-tests that referenced this pull request Apr 22, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@miq-bot
Copy link
Member

miq-bot commented Apr 22, 2020

Checked commit skateman@b4a8b33 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@Fryguy Fryguy merged commit e3e5907 into ManageIQ:master May 8, 2020
@skateman skateman deleted the rename-foreman-features branch May 8, 2020 18:47
simaishi pushed a commit that referenced this pull request May 21, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Renamed foreman features to ems_configuration and adjusted roles

(cherry picked from commit e3e5907)
@simaishi
Copy link
Contributor

Jansa backport details:

$ git log -1
commit c8a9c45aa717255e43022b7290011b47194c4986
Author: Jason Frey <[email protected]>
Date:   Thu May 7 22:05:16 2020 -0400

    Merge pull request #464 from skateman/rename-foreman-features

    Renamed foreman features to ems_configuration and adjusted roles

    (cherry picked from commit e3e59070665932b94e0b2a5bbd0bd28f7713b346)

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.

7 participants