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

Remove Settings.product.transformation - v2v no longer conditional on product feature #17711

Merged
merged 1 commit into from
Jul 20, 2018
Merged

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Jul 16, 2018

Depends on ManageIQ/manageiq-v2v#490 (and ManageIQ/manageiq-v2v#417). (Both merged now.)

Second part of the fix for ManageIQ/manageiq-v2v#432

Cc @Fryguy

@himdel himdel changed the title Remove Settings.product.transformation - v2v no longer conditional on… Remove Settings.product.transformation - v2v no longer conditional on product feature Jul 16, 2018
@miq-bot
Copy link
Member

miq-bot commented Jul 16, 2018

Checked commit https://github.com/himdel/manageiq/commit/130ee97a012c658f7e84f87ac463059d41734fda with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

@himdel
Copy link
Contributor Author

himdel commented Jul 16, 2018

@miq-bot add_label gaprindashvili/yes

@martinpovolny
Copy link
Member

Restarted travis.

@carbonin
Copy link
Member

Was this ever released? If so I think we would need a migration as well to remove this key from settings_changes right? /cc @Fryguy

@martinpovolny
Copy link
Member

I am not quite sure. It was meant for Gaprindashvili z-stream. @AllenBW, @AparnaKarve, can you answer @carbonin 's question, please?

@AparnaKarve
Copy link
Contributor

When these PRs #17270 and #17285 were created, the idea was to disable v2v feature in 5.9.3 by default and enable it on upstream. (/cc @gmcculloug )
So, a little surprised to see this PR...

But if the above requirement has changed, and if we indeed want to get rid of the Settings.product.transformation flag, then the PR looks good.

@AparnaKarve
Copy link
Contributor

If we intend to keep the flag for Gaprindashvili only, then this PR and ManageIQ/manageiq-v2v#490 need to be gaprindashvili/no

@gmcculloug gmcculloug self-assigned this Jul 17, 2018
@gmcculloug gmcculloug added the v2v label Jul 17, 2018
@gmcculloug
Copy link
Member

@AparnaKarve The feature is moving from preview to general availability on the gaprindashvili branch which is why the flag is no longer needed. Does that help?

@AparnaKarve
Copy link
Contributor

@gmcculloug Thanks - that helps.

In that case, the PR is GTG.

@Fryguy
Copy link
Member

Fryguy commented Jul 17, 2018

I agree with @carbonin that this requires a data migration to delete any records. Technically it doesn't hurt anything having an "extra" key in there, but if a choice was made and we are no longer honoring that choice, then it's potentially confusing for a user. Better to just delete the setting.

@himdel
Copy link
Contributor Author

himdel commented Jul 18, 2018

OK, created a data migration - ManageIQ/manageiq-schema#236

@AparnaKarve
Copy link
Contributor

hmm, a data migration for Gaprindashvili - I suppose we can make one exception?

For G, another option to consider would be to just flip the transformation flag to true
Advantage - no data migrations required in G

In discussion with @gmcculloug, he suggested this option - it's a great option if we don't want to deal with migrations in Gaprindashvili

@himdel
Copy link
Contributor Author

himdel commented Jul 18, 2018

IMO we don't need the migration in gaprindashvili, as long as we don't mind the extra key there.

But OTOH I don't see any harm in backporting that particular migration.

I don't have an opinion here.

@carbonin
Copy link
Member

@AparnaKarve @himdel There's no harm in backporting the migration, but even if we don't backport it, it's something that still needs to be done.

@gmcculloug
Copy link
Member

@Fryguy can correct me if I'm wrong, but I think the normal z-stream updates do not run migrations, so the migration file would be there but no one would run it.

@gmcculloug
Copy link
Member

@himdel The recommended approach would be create a PR on Gaprindashvili which changes the flag from false to true. Upstream we remove the setting, include the migration and remove the UI check for the flag.

At this point it looks like we just have to change the label on this PR and ManageIQ/manageiq-v2v#490 to gaprindashvili/no and create the Gaprindashvili PR to change the existing flag to true.

@himdel
Copy link
Contributor Author

himdel commented Jul 20, 2018

Aah, OK @gmcculloug , thanks :). On it.. #17733

@miq-bot add_label gaprindashvili/no
@miq-bot remove_label gaprindashvili/yes

@gmcculloug gmcculloug merged commit ece95c6 into ManageIQ:master Jul 20, 2018
@gmcculloug gmcculloug added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 20, 2018
@himdel himdel deleted the product-trans branch July 20, 2018 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants