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

Fixes #30544 - deprecate mount_react_component #7874

Merged

Conversation

sharvit
Copy link
Contributor

@sharvit sharvit commented Aug 3, 2020

Deprecate the ReactjsHelper#mount_react_component,
please use ReactjsHelper#react_component instead.

Please wait for the next develop version before merging.

@theforeman-bot
Copy link
Member

Issues: #30544

@tbrisker
Copy link
Member

tbrisker commented Aug 3, 2020

Would it make sense to first change the current places where this helper is used, at least in core, before marking it as deprecated?

@sharvit
Copy link
Contributor Author

sharvit commented Aug 3, 2020

Would it make sense to first change the current places where this helper is used, at least in core, before marking it as deprecated?

Yes, I will add individual issues for each instance and try to split the effort.

@ekohl
Copy link
Member

ekohl commented Aug 3, 2020

I think that individual Redmine issues will greatly pollute the changelog and make it much less clear what was the actual change. Can't you make a list and use Refs #xxx?

@tbrisker we have a similar thing in the installer and I'm wondering what you think is a best practice here.

@tbrisker
Copy link
Member

tbrisker commented Aug 3, 2020

I think that individual Redmine issues will greatly pollute the changelog and make it much less clear what was the actual change. Can't you make a list and use Refs #xxx?

@tbrisker we have a similar thing in the installer and I'm wondering what you think is a best practice here.

It makes sense to me in such cases where it's clear that there are multiple steps to get one issue resolved, on the other hand it is sometimes useful to break down a larger task into smaller ones that can be done separately - for example see https://projects.theforeman.org/issues/28278. So I would say that IMHO it depends on the size and complexity of the effort.

@sharvit
Copy link
Contributor Author

sharvit commented Aug 3, 2020

IMO the benefit of having individual Redmine issues is so developers can assign themself and we can split the effort.

@jeremylenz
Copy link
Contributor

You can make a Redmine tracker that brings together all the individual issues

@sharvit
Copy link
Contributor Author

sharvit commented Aug 13, 2020

You can make a Redmine tracker that brings together all the individual issues

Yes we have this one: https://projects.theforeman.org/issues/30544
Feel free to help 👍

@ezr-ondrej
Copy link
Member

I've rebased for you, let see what the tests are going to say now :)

@ezr-ondrej ezr-ondrej self-assigned this Sep 30, 2020
@ezr-ondrej
Copy link
Member

There seems to be one more, that slipped our attention: #8040

@MariaAga
Copy link
Member

MariaAga commented Oct 4, 2020

There are plugins that also use mount_react_component , I opened redmines:
https://projects.theforeman.org/issues/30969
https://projects.theforeman.org/issues/30970
https://projects.theforeman.org/issues/30971
https://projects.theforeman.org/issues/30972
theforeman/foreman_puppet#36

@LaViro Not sure where you track issues in rh-cloud but there are a few uses there: https://github.com/theforeman/foreman_rh_cloud/search?q=mount_react_component

@Ron-Lavi
Copy link
Member

Ron-Lavi commented Oct 4, 2020

@LaViro Not sure where you track issues in rh-cloud but there are a few uses there: https://github.com/theforeman/foreman_rh_cloud/search?q=mount_react_component

We also support older versions and its easier for us to do the CPs,
although I guess we could update the master branch to use react_component instead
cc @ShimShtein

On what foreman version would it be deprecated?

@ezr-ondrej
Copy link
Member

On what foreman version would it be deprecated?

I hope we can make it in foreman 2.3.

@tbrisker
Copy link
Member

tbrisker commented Oct 5, 2020

The react_component helper was introduced in https://projects.theforeman.org/issues/26735 which is 2.0.

@@ -1,5 +1,6 @@
module ReactjsHelper
def mount_react_component(name, selector, data = [], opts = {})
Foreman::Deprecation.deprecation_warning('2.5', 'use #react_component instead #mount_react_component')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Foreman::Deprecation.deprecation_warning('2.5', 'use #react_component instead #mount_react_component')
Foreman::Deprecation.deprecation_warning('2.5', 'use #react_component instead of #mount_react_component')

Copy link
Member

Choose a reason for hiding this comment

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

I've applied this.

@Ron-Lavi
Copy link
Member

Ron-Lavi commented Oct 5, 2020

Opened theforeman/foreman_rh_cloud#357 to refactor it in foreman_rh_cloud

@ezr-ondrej
Copy link
Member

I've rebased again, let see 👍

Deprecate the ReactjsHelper#mount_react_component,
please use ReactjsHelper#react_component instead.
@ezr-ondrej ezr-ondrej force-pushed the ref/deprecate_mount_react_component branch from 88263ae to e9ea416 Compare October 21, 2020 12:07
@ezr-ondrej
Copy link
Member

I've added Katello ticket: https://projects.theforeman.org/issues/31122 with PR, that leaves us only with OpenSCAP plugin in our organization. I think we are ready to go 👍

Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

I believe this is ready to get in 👍
Thanks @sharvit for kicking this off, thanks @MariaAga, @amirfefer, @xprazak2, @LaViro and @jeremylenz for the joined effort to get rid of the old mounter usage.

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

thanks @sharvit @ezr-ondrej and everyone involved in this effort!

@tbrisker tbrisker merged commit 799363a into theforeman:develop Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants