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

Add foreman_acd #4388

Merged
merged 1 commit into from Jun 29, 2020
Merged

Add foreman_acd #4388

merged 1 commit into from Jun 29, 2020

Conversation

sbernhard
Copy link

No description provided.

BuildRequires: %{?scl_prefix}npm(jquery) < 2.3.0
BuildRequires: %{?scl_prefix}npm(react-intl) >= 2.8.0
BuildRequires: %{?scl_prefix}npm(react-intl) < 3.0.0
# end package.json dependencies BuildRequires
Copy link
Member

Choose a reason for hiding this comment

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

Not that this blocks, but please consider moving to use of the foreman vendor JS.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @ehelms for your remark. I had a look at other plugins and no-one is currently using it (unfortunately).
Should I then ONLY use the foreman-vendor package and remove all other npm packages or should I add foreman-vendor and still use the npm packages?

Copy link
Member

Choose a reason for hiding this comment

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

Plugins like foreman_ansible are using it. They also have additional dependencies. Probably best to ask the UI team. I'm sure @sharvit will happily guide you in the right direction.

Copy link

Choose a reason for hiding this comment

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

The build process will replace all the modules you are using with the modules that exist in the vendor anyway. (if they exist in the vendor)

It's a good practice to install the vendor as a dep and clean up modules from your package.json so your plugin declares what vendor version it needs. It will help with upgrading once there are breaking changes to the vendor since you will know about them in advance.

@sbernhard
Copy link
Author

Thanks @sharvit. Means, remove all dependencies and replace them with just and only foreman-vendor?

@sharvit
Copy link

sharvit commented Nov 23, 2019

Thanks @sharvit. Means, remove all dependencies and replace them with just and only foreman-vendor?

Only if they exist in the vendor, see: https://github.com/theforeman/foreman-js/blob/master/packages/vendor-core/package.json#L27

@sbernhard
Copy link
Author

Thanks. BTW, jQuery is really old and has some security issues (says GitHub)

@sharvit
Copy link

sharvit commented Nov 25, 2019

Thanks. BTW, jQuery is really old and has some security issues (says GitHub)

Yes, actually we want to be in a point where we can drop jquery entierly. (I would recommend avoiding using jquery if possible)
See:

@sbernhard sbernhard force-pushed the add_foreman_acd branch 2 times, most recently from b7b102c to 8c94777 Compare November 25, 2019 09:01
@sbernhard
Copy link
Author

Thanks @sharvit for your explanation. I will have a look how to get rid of jquery (soon).

Currently, the build RPM tasks failes because of:
http://koji.katello.org/koji/getfile?taskID=252074&volume=DEFAULT&name=build.log&offset=-4000

Is it related to different version of lodash in https://github.com/theforeman/foreman-js/blob/f0e3fe1d27255f77e2a6303185ea3e578c9845a4/packages/vendor-core/package.json#L50 and

?

@sharvit
Copy link

sharvit commented Nov 25, 2019

It might be that but I don't think it's related since cloneDeep exists in lodash since v1

@sbernhard
Copy link
Author

hmm. Do you have another idea whats wrong? Previously. without using foreman-vendor-js the build was successfully (used lodash 4.17.11).

@sharvit
Copy link

sharvit commented Nov 25, 2019

hmm. Do you have another idea whats wrong? Previously. without using foreman-vendor-js the build was successfully (used lodash 4.17.11).

  • Do you have the same issue when building locally against @theforeman/vendor (v1.4)?
  • Do you have the same issue when building locally against lodash v4.17.11?

@sbernhard sbernhard force-pushed the add_foreman_acd branch 4 times, most recently from b05dcc2 to 7e9883f Compare November 25, 2019 14:48
@sbernhard
Copy link
Author

Package build was successfully after:

  • adding lodash 4.17.11
  • adding patternfly (because of some necesary variables)
  • adding patternfly-react (because of _inline_edit variables)

@sharvit
Copy link

sharvit commented Nov 25, 2019

Package build was successfully after:

  • adding lodash 4.17.11

Very odd.

It might happen if you are importing partials from lodash (e.g. import something from 'lodash/something') but not if you import all lodash (e.g. import { something } from 'lodash' wich is the right way)

  • adding patternfly (because of some necesary variables)
  • adding patternfly-react (because of _inline_edit variables)

You shouldn't import patternfly in your scss files to get their variables, instead, you should import them from the vendor.
See: https://github.com/theforeman/foreman-js/tree/master/packages/vendor#stylesheets

@sbernhard
Copy link
Author

I can the _variables -> foreman-js/variables import but I can not find the
_loading-state and the _inline_edit. Are they not included in foreman-js? Do I need to use them from patternfly and patternfly-react?

@sharvit
Copy link

sharvit commented Nov 26, 2019

You should not load them internally, all the pf partials should exist in https://github.com/theforeman/foreman-js/blob/master/packages/vendor-core/scss/vendor-core.scss

If they are not, we should create a PR and add them to the vendor-core.scss file.
The vendor-core.scss is the opinionated 3rd-party css bundle that we deliver to plugins.
Other than that you can also use the 3rd-party variables and mixins from: https://github.com/theforeman/foreman-js/tree/master/packages/vendor#stylesheets

@sbernhard sbernhard force-pushed the add_foreman_acd branch 3 times, most recently from c637b59 to 4e26d08 Compare November 27, 2019 09:49
@sbernhard
Copy link
Author

Removed patternfly-(react) dependencies. Unfortunately, lodash must still be a dependency - otherwise it doesn't build.

@ehelms
Copy link
Member

ehelms commented Jan 15, 2020

[test rpm]

2 similar comments
@zjhuntin
Copy link
Contributor

[test rpm]

@ehelms
Copy link
Member

ehelms commented Mar 10, 2020

[test rpm]

@ehelms
Copy link
Member

ehelms commented Apr 15, 2020

[test rpm]

@ehelms
Copy link
Member

ehelms commented Apr 15, 2020

@sbernhard Needs updates to latest dependencies

@pcreech pcreech merged commit b073696 into theforeman:rpm/develop Jun 29, 2020
@nadjaheitmann nadjaheitmann deleted the add_foreman_acd branch February 16, 2024 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants