Update foreman package dependencies #3070

ekohl · 2018-10-08T10:38:01Z

No description provided.

ohadlevy · 2018-10-08T11:11:17Z

could you also add nodejs-jed as a dependency ? imho theforeman/foreman#5342 will be merged today.

ekohl · 2018-10-08T14:29:55Z

It fails with a compilation error that I can't quite explain:

ERROR in ./webpack/assets/javascripts/react_app/components/AutoComplete/auto-complete.scss
Module build failed: ModuleNotFoundError: Module not found: Error: Can't resolve 'react-bootstrap-typeahead/css/Typeahead.css' in '/builddir/build/BUILD/foreman-1.20.0-develop/webpack/assets/javascripts/react_app/components/AutoComplete'
    at factoryCallback (/usr/lib/node_modules/webpack/lib/Compilation.js:282:40)
    at factory (/usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:237:20)
    at resolver (/usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:60:20)
    at asyncLib.parallel.e (/usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:127:20)
    at /usr/lib/node_modules/webpack/node_modules/async/dist/async.js:3888:9
    at /usr/lib/node_modules/webpack/node_modules/async/dist/async.js:473:16
    at iteratorCallback (/usr/lib/node_modules/webpack/node_modules/async/dist/async.js:1062:13)
    at /usr/lib/node_modules/webpack/node_modules/async/dist/async.js:969:16
    at /usr/lib/node_modules/webpack/node_modules/async/dist/async.js:3885:13
    at resolvers.normal.resolve (/usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:119:22)
    at onError (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:65:10)
    at loggingCallbackWrapper (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/createInnerCallback.js:31:19)
    at runAfter (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:158:4)
    at innerCallback (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:146:3)
    at loggingCallbackWrapper (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/createInnerCallback.js:31:19)
    at next (/usr/lib/node_modules/webpack/node_modules/tapable/lib/Tapable.js:252:11)
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/UnsafeCachePlugin.js:40:4
    at loggingCallbackWrapper (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/createInnerCallback.js:31:19)
    at runAfter (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:158:4)
    at innerCallback (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:146:3)
    at loggingCallbackWrapper (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/createInnerCallback.js:31:19)
    at next (/usr/lib/node_modules/webpack/node_modules/tapable/lib/Tapable.js:252:11)
    at innerCallback (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:144:11)
    at loggingCallbackWrapper (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/createInnerCallback.js:31:19)
    at next (/usr/lib/node_modules/webpack/node_modules/tapable/lib/Tapable.js:249:35)
    at resolver.doResolve.createInnerCallback (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:44:6)
    at loggingCallbackWrapper (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/createInnerCallback.js:31:19)
    at afterInnerCallback (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:166:11)
    at loggingCallbackWrapper (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/createInnerCallback.js:31:19)
    at next (/usr/lib/node_modules/webpack/node_modules/tapable/lib/Tapable.js:249:35)
 @ ./webpack/assets/javascripts/react_app/components/AutoComplete/index.js 51:0-31
 @ ./webpack/assets/javascripts/react_app/components/SearchBar/SearchBar.js
 @ ./webpack/assets/javascripts/react_app/components/SearchBar/index.js
 @ ./webpack/assets/javascripts/react_app/components/componentRegistry.js
 @ ./webpack/assets/javascripts/react_app/common/MountingService.js
 @ ./webpack/assets/javascripts/bundle.js

Ron-Lavi · 2018-10-08T14:52:38Z

Interesting.. I will look more into it tomorrow :)

ohadlevy · 2018-10-08T14:52:46Z

any chance this is similar to theforeman/foreman#6043 ? /cc @sharvit

tbrisker

Lgtm

tbrisker

Oops, on mobile, didn't notice the failing build

ohadlevy · 2018-10-08T14:57:09Z

@ekohl does the node_modules directory contain node_modules/react-bootstrap-typeahead/css/Typeahead.css ?

ekohl · 2018-10-08T14:59:32Z

@ohadlevy no, it's an indirect dependency that's bundled via patternfly.

ohadlevy · 2018-10-09T06:07:24Z

@ekohl where can I find the resulting node_modules directory? is it available on koji somewhere?

Ron-Lavi · 2018-10-09T08:28:34Z

react-bootstrap-typeahead/css/Typeahead.css
is being added to patternfly TypeaheadSelect and seem to work there,
maybe it is something with foreman css-loaders ?

There is a small diff between patternfly and foreman scss loaders, but I am not sure it is causing the problem:

Foreman:

      test: /\.css$/,
      loaders: ['style-loader', 'css-loader']
    },
    {
      test: /\.scss$/,
      loaders: ['style-loader', 'css-loader', 'sass-loader']
    },

Patternfly:

      test: /\.css$/,
      loaders: ['style-loader', 'css-loader'],
     ...
    },
    // Sass
    {
      test: /\.scss$/,
      use: [
        { loader: 'style-loader' },
        { loader: 'css-loader' },
        ...
      ]
    }

tbrisker · 2018-10-09T08:32:56Z

looks like thie is the offensive code: https://github.com/patternfly/patternfly-react/blob/master/packages/patternfly-3/patternfly-react/sass/patternfly-react/_type-ahead-select.scss#L1 - patternfly-react seems to expect that on the top level.
if we can't fix it by tomorrow i'd rather we revert the search change and postpone it to after branching rather than delay branching or branch with broken code.

ohadlevy · 2018-10-09T08:45:20Z

On Tue, Oct 9, 2018 at 11:32 AM Tomer Brisker ***@***.***> wrote: looks like thie is the offensive code: https://github.com/patternfly/patternfly-react/blob/master/packages/patternfly-3/patternfly-react/sass/patternfly-react/_type-ahead-select.scss#L1 - patternfly-react seems to expect that on the top level. if we can't fix it by tomorrow i'd rather we revert the search change and postpone it to after branching rather than delay branching or branch with broken code.

in that case, can't we simply package it as a top level?

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3070 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOx07KMcyX4NtmhHtIryAAU340viwRks5ujF85gaJpZM4XMqsV> .

ohadlevy · 2018-10-09T08:46:40Z

@tbrisker I find it odd, as patternfly require a lot of packages, and so far we had no problems with yet, I assume here there is something to do with how the node_modules we generate looks like using packages, if we have a way to get a sample of it, maybe we can figure out whats exactly wrong. regardless, I would hope just packaging it as top level can solve the issue?

ekohl · 2018-10-09T09:21:40Z

I do think packaging it at the top level would be a short term fix. Previously the package was unbundled so perhaps that's why we weren't seeing it. Perhaps the ~package import always expects things to be at the top level though I wonder why that isn't the case with a npm install.

ekohl · 2018-10-09T09:31:13Z

I stand corrected: after npm install it does show at the top level of node_modules. It looks like npm prefers to install at the top level but only bundle if there's a conflict. I've tried to keep as much packages unbundled because I thought it was cleaner. Looks like that approach is closer to matching what npm does already.

ohadlevy · 2018-10-09T09:40:21Z

@ekohl are you suggesting the package is on the top level already? (and that webpack can't find it regardless?)
@tbrisker another work around we can do is simply copying the relevant css to our code, and drop the import statement, removing the entire search feature imho should be last resort... :(

ekohl · 2018-10-09T09:43:13Z

@ekohl are you suggesting the package is on the top level already? (and that webpack can't find it regardless?)

No, I'm saying we have most packages at top level but with patternfly we switched from unbundled to bundled in the latest update. That's why we might be seeing this now and not before.

ohadlevy · 2018-10-09T09:46:46Z

No, I'm saying we have most packages at top level but with patternfly we switched from unbundled to bundled in the latest update. That's why we might be seeing this now and not before.

so in this case, maybe even reverting won't help as its just the first error out of a few others...?

tbrisker · 2018-10-09T10:43:04Z

OK, what is the next step than? Should we return patternfly-react to be unbundled?

Ron-Lavi · 2018-10-09T11:29:27Z

In case we don't want the SearchBar styles to block this PR
and until we decide whether to change patternfly-react bundle or not,
I've created this PR,
where we save locally that css file we need to import.

ekohl · 2018-10-09T11:39:11Z

OK, what is the next step than? Should we return patternfly-react to be unbundled?

I am leaning to that yes.

tbrisker · 2018-10-09T12:16:09Z

@ekohl let's do that then, we need to have working nightlies prior to branching.

ekohl · 2018-10-10T16:42:47Z

For completeness: #3077

ekohl · 2018-10-10T17:21:19Z

When https://ci.theforeman.org/job/foreman-packaging-release/147/ is done the tests can be restarted.

tbrisker · 2018-10-10T18:44:45Z

[test rpm]

ekohl · 2018-10-10T18:59:49Z

Looks like our repoclosure check doesn't take repositories into account. nodejs-invariant is in the katello repository but Foreman can't use that. Going to do some shuffling

ekohl · 2018-10-10T19:01:42Z

I stand corrected, it was a release issue. foreman-packaging-release can't deal with a package already existing and http://koji.katello.org/koji/buildinfo?buildID=27723 was already built. It was skipped but is tagged to the wrong repo. During the PR it was built locally as a scratch and so repoclosure passed.

ekohl · 2018-10-10T19:04:12Z

[test rpm]

ekohl · 2018-10-10T19:15:25Z

[test rpm]

ekohl · 2018-10-10T19:43:10Z

Since it's green now, I'm going to merge this. Haven't verified the actual in UI functionality but it'll at least unblock our builds.

tbrisker · 2018-10-10T19:50:47Z

Thanks for all the hard effort @ekohl !

ohadlevy · 2018-10-10T20:06:52Z

+1 thanks!

…

On Wed, Oct 10, 2018, 10:50 PM Tomer Brisker ***@***.***> wrote: Thanks for all the hard effort @ekohl <https://github.com/ekohl> ! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3070 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOx6t3jCcSRZBTBg_IXmdlWwIMFurqks5ujk-YgaJpZM4XMqsV> .

theforeman-bot added Needs testing Not yet reviewed RPM labels Oct 8, 2018

ekohl mentioned this pull request Oct 8, 2018

Refs #24874 - update secure_headers dependency to 6.x #2953

Closed

1 task

ekohl mentioned this pull request Oct 8, 2018

Bump nodejs-patternfly-react to 2.19.1 #3063

Merged

Update foreman package dependencies

e607669

ekohl force-pushed the rpm/update-foreman-deps branch from fa04f35 to e607669 Compare October 8, 2018 14:13

tbrisker approved these changes Oct 8, 2018

View reviewed changes

theforeman-bot removed the Not yet reviewed label Oct 8, 2018

tbrisker suggested changes Oct 8, 2018

View reviewed changes

theforeman-bot added the Waiting on contributor label Oct 8, 2018

Ron-Lavi mentioned this pull request Oct 9, 2018

Fixes #25153 - AutoComplete can't import styles due to bundle issue theforeman/foreman#6125

Closed

tbrisker mentioned this pull request Oct 10, 2018

Update foreman_remote_execution to 1.6.4 [rpm] #3075

Merged

4 tasks

ekohl merged commit 92e73aa into theforeman:rpm/develop Oct 10, 2018

ekohl deleted the rpm/update-foreman-deps branch October 10, 2018 19:43

Update foreman package dependencies #3070

Update foreman package dependencies #3070

Uh oh!

Conversation

ekohl commented Oct 8, 2018

Uh oh!

ohadlevy commented Oct 8, 2018

Uh oh!

ekohl commented Oct 8, 2018

Uh oh!

Ron-Lavi commented Oct 8, 2018

Uh oh!

ohadlevy commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tbrisker left a comment

Choose a reason for hiding this comment

Uh oh!

tbrisker left a comment

Choose a reason for hiding this comment

Uh oh!

ohadlevy commented Oct 8, 2018

Uh oh!

ekohl commented Oct 8, 2018

Uh oh!

ohadlevy commented Oct 9, 2018

Uh oh!

Ron-Lavi commented Oct 9, 2018

Uh oh!

tbrisker commented Oct 9, 2018

Uh oh!

ohadlevy commented Oct 9, 2018 via email

Uh oh!

ohadlevy commented Oct 9, 2018

Uh oh!

ekohl commented Oct 9, 2018

Uh oh!

ekohl commented Oct 9, 2018

Uh oh!

ohadlevy commented Oct 9, 2018

Uh oh!

ekohl commented Oct 9, 2018

Uh oh!

ohadlevy commented Oct 9, 2018

Uh oh!

tbrisker commented Oct 9, 2018

Uh oh!

Ron-Lavi commented Oct 9, 2018

Uh oh!

ekohl commented Oct 9, 2018

Uh oh!

tbrisker commented Oct 9, 2018

Uh oh!

ekohl commented Oct 10, 2018

Uh oh!

ekohl commented Oct 10, 2018

Uh oh!

tbrisker commented Oct 10, 2018

Uh oh!

ekohl commented Oct 10, 2018

Uh oh!

ekohl commented Oct 10, 2018

Uh oh!

ekohl commented Oct 10, 2018

Uh oh!

ekohl commented Oct 10, 2018

Uh oh!

ekohl commented Oct 10, 2018

Uh oh!

tbrisker commented Oct 10, 2018

Uh oh!

ohadlevy commented Oct 10, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ohadlevy commented Oct 8, 2018 •

edited

Loading