Display a deprecated message when Service uses plugin deployment #1391

duduribeiro · 2019-10-31T15:53:56Z

Our plugin deployment option is deprecated. To avoid needing some data migration in database, this commit sets the deployment option as hosted if the current option is set to plugins and also displays a message to the user to update their configuration.

codecov · 2019-10-31T19:07:05Z

Codecov Report

Merging #1391 into master will increase coverage by 2.28%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1391      +/-   ##
==========================================
+ Coverage   89.77%   92.05%   +2.28%     
==========================================
  Files        2185     2392     +207     
  Lines       67076    76259    +9183     
==========================================
+ Hits        60215    70203    +9988     
+ Misses       6861     6056     -805

Impacted Files	Coverage Δ
test/unit/helpers/services_helper_test.rb	`100% <ø> (ø)`	⬆️
app/models/service.rb	`93.63% <100%> (+0.11%)`	⬆️
test/unit/service_test.rb	`100% <100%> (ø)`	⬆️
...developer_portal/admin/account/ogone_controller.rb	`93.75% <0%> (-6.25%)`	⬇️
app/models/forum.rb	`88.88% <0%> (-0.4%)`	⬇️
...user-management-api/services/mapping_rules_test.rb	`100% <0%> (ø)`	⬆️
app/lib/backend_api_logic/routing_policy.rb	`100% <0%> (ø)`	⬆️
...loper_portal/buyer/account_contracts_controller.rb	`91.66% <0%> (ø)`	⬆️
...ration/buyers/service_contracts_controller_test.rb	`100% <0%> (ø)`	⬆️
...developer_portal/admin/accounts_controller_test.rb	`100% <0%> (ø)`	⬆️
... and 997 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14c3334...9f6d191. Read the comment docs.

hallelujah · 2019-11-05T07:38:42Z

app/models/service.rb

+
+  # TODO: Remove this when no one use plugins
+  def deployment_option
+    return 'hosted' if plugin_deployment?


I think it should be self_managed
People using plugin do not really have any apicast in front. It would enforce them to have one hosted
Also zync will create a route automatically in OpenShift if it is hosted

I’d say the logical setting is what you get out of the box when you install 3scale. That would be 3scale managed. Other question is if it would work.

OK but anyway there are many references to deployment_option in the code
I'd prefer just updating the database and remove the plugins from the code.

For example, I do not know if we will show the policies or the mapping rules because of this change.

This is an effort to have something for 2.7.

Yes, everything is shown as if 3scale managed APIcast was being used. And when the customer clicks ok, setting will actually be updated in db. I agree this is not the final solution but it is kind of a nice in between with heads up to customer. Next step is remove all of it.

We can remove the code later but a data migration (DML) should be done IMO
Anyway plugins are not shown anymore right?

They are still shown (I just verified), deprecation notice was already issued long time ago. This is adding more work and has really unknown behaviours

Also this undesirable behaviour is worse than leaving it there: Zync will just create routes if it is hosted unlike before while it was still plugin.

If we just display the message and not touch the deployment_option method I would merge the PR.

Why is it wrong that zync creates the routes? If they change to apicast because they want to that would also happen, no?

It is a change of behaviour without notice zync will create tons of routes ...
Not to mention that some users reported that already 3scale/zync#236

If they want to use the hosted gateway yes, but they might also want to use the self managed gateway or the service mesh.

Agree that it makes sense to have default to hosted but I'd say the timing is pretty bad to take this decision ...

EDIT: in fact I would make it self_managed instead of hosted because plugins are in a sense self managed by the customer.

hallelujah · 2019-11-05T07:39:07Z

app/views/shared/service/_deprecated_plugin_warning.html.slim

@@ -0,0 +1,7 @@
+- if @service&.plugin_deployment?


I do not get why the nil check

There were some controllers Api::MetricsController that used this layout but didn't loaded the service in the variable.

This smells then :)

josemigallas · 2019-11-05T07:26:18Z

app/models/service.rb

+  # TODO: Remove this when no one use plugins
+  def plugin_deployment?
+    deployment = self[:deployment_option]
+    DeploymentOption.plugins.include?(deployment)


And here I thought Rubysts were allergic to parenthesis!

Suggested change

DeploymentOption.plugins.include?(deployment)

DeploymentOption.plugins.include? deployment

😂 .. I hate to omit the parenthesis

app/views/layouts/api/_service.html.slim

guicassolato

Let's make this PR about updating the deprecation message only and send another one for the actual removal of the code, which may not fit 2.7 release.

hallelujah

Please just show the message and do not touch the deployment_option method.

Martouta · 2019-11-06T15:14:00Z

Quoted from Hery:

OK for me we would need to:

Merge you PR with only the notice Display a deprecated message when Service uses plugin deployment #1391

Merge my PR removing the plugins in the api and UI [Rolling Updates] Plugin deployment option are deprecated #1249

Fix https://github.com/3scale/porta/blob/master/app/models/proxy.rb#L121 by migrating all deployment_option from service to Proxy

Remove the column from Service with the code first then with a real migration

make a DML task to put all plugins to hosted or self_managed TBD

Remove plugin code

THREESCALE-2395 Our plugin deployment option is deprecated. To avoid needing some data migration in database, this commit sets the deployment option as hosted if the current option is set to plugins and also displays a message to the user to update their configuration.

duduribeiro force-pushed the add_warning_for_plugin_deployment branch 2 times, most recently from 9d47b95 to 0a41e06 Compare October 31, 2019 19:06

duduribeiro changed the title ~~[wip]~~ Display a deprecated message when Service uses plugin deployment Oct 31, 2019

duduribeiro force-pushed the add_warning_for_plugin_deployment branch from 0a41e06 to f6c5bef Compare October 31, 2019 19:09

duduribeiro requested review from thomasmaas and a team October 31, 2019 19:10

duduribeiro self-assigned this Oct 31, 2019

duduribeiro marked this pull request as ready for review October 31, 2019 19:11

duduribeiro force-pushed the add_warning_for_plugin_deployment branch from f6c5bef to a8a2f91 Compare November 1, 2019 14:06

duduribeiro changed the title ~~Display a deprecated message when Service uses plugin deployment~~ [wip] Display a deprecated message when Service uses plugin deployment Nov 4, 2019

thomasmaas force-pushed the add_warning_for_plugin_deployment branch from c1d0fb8 to 9c3db75 Compare November 4, 2019 20:14

hallelujah reviewed Nov 5, 2019

View reviewed changes

josemigallas reviewed Nov 5, 2019

View reviewed changes

thomasmaas changed the title ~~[wip] Display a deprecated message when Service uses plugin deployment~~ Display a deprecated message when Service uses plugin deployment Nov 5, 2019

guicassolato previously approved these changes Nov 5, 2019

View reviewed changes

macejmic previously approved these changes Nov 6, 2019

View reviewed changes

hallelujah suggested changes Nov 6, 2019

View reviewed changes

duduribeiro and others added 4 commits December 4, 2019 17:20

minimal height form

aed69cf

Only display the message instead of changing the deployment_option

9f6d191

duduribeiro dismissed stale reviews from macejmic and guicassolato via 9f6d191 December 4, 2019 20:27

duduribeiro force-pushed the add_warning_for_plugin_deployment branch from 136fb60 to 9f6d191 Compare December 4, 2019 20:27

hallelujah approved these changes Dec 13, 2019

View reviewed changes

duduribeiro merged commit 6302212 into master Dec 13, 2019

duduribeiro deleted the add_warning_for_plugin_deployment branch December 13, 2019 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display a deprecated message when Service uses plugin deployment #1391

Display a deprecated message when Service uses plugin deployment #1391

duduribeiro commented Oct 31, 2019 •

edited by thomasmaas

codecov bot commented Oct 31, 2019 •

edited

hallelujah Nov 5, 2019

thomasmaas Nov 5, 2019

hallelujah Nov 5, 2019

thomasmaas Nov 5, 2019

thomasmaas Nov 5, 2019

hallelujah Nov 6, 2019

hallelujah Nov 6, 2019

thomasmaas Nov 6, 2019

hallelujah Nov 6, 2019 •

edited

hallelujah Nov 5, 2019

duduribeiro Nov 5, 2019

hallelujah Nov 6, 2019

josemigallas Nov 5, 2019

duduribeiro Nov 5, 2019

guicassolato left a comment

hallelujah left a comment

Martouta commented Nov 6, 2019

	DeploymentOption.plugins.include?(deployment)
	DeploymentOption.plugins.include? deployment

Display a deprecated message when Service uses plugin deployment #1391

Display a deprecated message when Service uses plugin deployment #1391

Conversation

duduribeiro commented Oct 31, 2019 • edited by thomasmaas

codecov bot commented Oct 31, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hallelujah Nov 6, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guicassolato left a comment

Choose a reason for hiding this comment

hallelujah left a comment

Choose a reason for hiding this comment

Martouta commented Nov 6, 2019

duduribeiro commented Oct 31, 2019 •

edited by thomasmaas

codecov bot commented Oct 31, 2019 •

edited

hallelujah Nov 6, 2019 •

edited