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 #37664 - switch to using js-cookie #10250

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

MariaAga
Copy link
Member

replaces jquery.cookie with js-cookie, as jquery.cookie has moved to js-cookie

@MariaAga MariaAga requested a review from a team as a code owner July 18, 2024 18:54
@github-actions github-actions bot added UI Legacy JS PRs making changes in the legacy Javascript stack labels Jul 18, 2024
@MariaAga
Copy link
Member Author

Should I try to create a packaging PR as well for this? @theforeman/packaging

@@ -81,7 +82,7 @@ function onContentLoad() {

tfm.i18n.intl.ready.then(function() {
var tz = jstz.determine();
$.cookie('timezone', tz.name(), {
Cookies.set('timezone', tz.name(), {
Copy link
Member

Choose a reason for hiding this comment

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

In other places we use tfm to pull in code from webpack. If we do that, I think we can avoid the Ruby dependency on js_cookie_rails.

I actually wonder why we need the timezone cookie. Some of my digging.

It was added in ad998ce. There it has set_timezone in app/controllers/concerns/application_shared.rb but that was moved to app/controllers/concerns/foreman/controller/timezone.rb in d1e1858. That makes me think it's still used. A quick look on the internet tells me this design is still a best practice.

Having said that: tfm.i18n.intl.ready looks to be coming from webpack code so you can also consider moving the whole cookie setting code to webpack.

Copy link
Member Author

Choose a reason for hiding this comment

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

its used in rubys set_timezone.
I moved it to webpack

bundler.d/assets.rb Outdated Show resolved Hide resolved
ekohl
ekohl previously approved these changes Jul 19, 2024
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

If you want to take a stab at packaging then please

@MariaAga
Copy link
Member Author

Added packaging pr theforeman/foreman-packaging#11048

@MariaAga
Copy link
Member Author

Added it to tfm as well as some plugins are using it

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Did you have a look at the test failure? I think I've seen that one pop up in multiple tests but haven't looked into it yet.

@MariaAga
Copy link
Member Author

Yeah I see test_0002_correctly override global params fail on some prs and cant reproduce it locally

@ekohl
Copy link
Member

ekohl commented Jul 23, 2024

I bet it'll turn out to be some nasty race condition or theforeman/actions#53

@MariaAga
Copy link
Member Author

MariaAga commented Sep 3, 2024

Rebased

Copy link
Contributor

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

Ran npm install and tested

_ForemanSelectedhosts cookie value is updated as expected. LGTM 👍

@adamruzicka
Copy link
Contributor

/packit build

@adamruzicka
Copy link
Contributor

The packit build failures will probably need more changes here

+ /usr/bin/rake webpack:compile DATABASE_URL=nulldb://nohost
2024-10-22T07:28:51 [I|app|] Rails cache backend: File
2024-10-22T07:28:51 [W|app|] You are trying to replace import_subnets from . Adding allowed actions from plugin permissions to the existing one.
npx --max_old_space_size=2048 webpack --config /builddir/build/BUILD/foreman-3.13.0/config/webpack.config.js --bail
[webpack-cli] ModuleNotFoundError: Module not found: Error: Can't resolve 'js-cookie' in '/builddir/build/BUILD/foreman-3.13.0/webpack/assets/javascripts'
    at /usr/lib/node_modules/webpack/lib/Compilation.js:2029:28
    at /usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:817:13
    at eval (eval at create (/usr/lib/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:10:1)
    at /usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:275:22
    at eval (eval at create (/usr/lib/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:9:1)
    at /usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:448:22
    at /usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:118:11
    at /usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:689:25
    at /usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:893:8
    at /usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:1013:5
    at /usr/lib/node_modules/webpack/node_modules/neo-async/async.js:6883:13
    at /usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:996:45
    at finishWithoutResolve (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:372:11)
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:461:15
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:519:5
    at eval (eval at create (/usr/lib/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:15:1)
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:519:5
    at eval (eval at create (/usr/lib/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:27:1)
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:89:43
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:519:5
    at eval (eval at create (/usr/lib/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:16:1)
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/forEachBail.js:39:13
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/AliasPlugin.js:148:14
    at next (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/forEachBail.js:35:3)
    at forEachBail (/usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/forEachBail.js:49:9)
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/AliasPlugin.js:61:5
    at _next0 (eval at create (/usr/lib/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:8:1)
    at eval (eval at create (/usr/lib/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:30:1)
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:519:5
    at eval (eval at create (/usr/lib/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:16:1)
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:519:5
    at eval (eval at create (/usr/lib/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:16:1)
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:519:5
    at eval (eval at create (/usr/lib/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:27:1)
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:89:43
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:519:5
    at eval (eval at create (/usr/lib/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:16:1)
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/Resolver.js:519:5
    at eval (eval at create (/usr/lib/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:15:1)
    at /usr/lib/node_modules/webpack/node_modules/enhanced-resolve/lib/DirectoryExistsPlugin.js:41:15
    at process.processTicksAndRejections (node:internal/process/task_queues:81:21)
resolve 'js-cookie' in '/builddir/build/BUILD/foreman-3.13.0/webpack/assets/javascripts'
  Parsed request is a module
  using description file: /builddir/build/BUILD/foreman-3.13.0/package.json (relative path: ./webpack/assets/javascripts)
    Field 'browser' doesn't contain a valid alias configuration
    resolve as module
      /builddir/build/BUILD/foreman-3.13.0/webpack/assets/javascripts/node_modules doesn't exist or is not a directory
      /builddir/build/BUILD/foreman-3.13.0/webpack/assets/node_modules doesn't exist or is not a directory
      /builddir/build/BUILD/foreman-3.13.0/webpack/node_modules doesn't exist or is not a directory
      looking for modules in /builddir/build/BUILD/foreman-3.13.0/node_modules
        single file module
          using description file: /builddir/build/BUILD/foreman-3.13.0/package.json (relative path: ./node_modules/js-cookie)
            no extension
              Field 'browser' doesn't contain a valid alias configuration
              /builddir/build/BUILD/foreman-3.13.0/node_modules/js-cookie doesn't exist
            .js
              Field 'browser' doesn't contain a valid alias configuration
              /builddir/build/BUILD/foreman-3.13.0/node_modules/js-cookie.js doesn't exist
            .json
              Field 'browser' doesn't contain a valid alias configuration
              /builddir/build/BUILD/foreman-3.13.0/node_modules/js-cookie.json doesn't exist
            .wasm
              Field 'browser' doesn't contain a valid alias configuration
              /builddir/build/BUILD/foreman-3.13.0/node_modules/js-cookie.wasm doesn't exist
        /builddir/build/BUILD/foreman-3.13.0/node_modules/js-cookie doesn't exist
      /builddir/build/BUILD/node_modules doesn't exist or is not a directory
      /builddir/build/node_modules doesn't exist or is not a directory
      /builddir/node_modules doesn't exist or is not a directory
      /node_modules doesn't exist or is not a directory
rake aborted!
Command failed with status (2): [npx --max_old_space_size=2048 webpack --co...]
/builddir/build/BUILD/foreman-3.13.0/lib/tasks/webpack_compile.rake:18:in `block (2 levels) in <top (required)>'
/usr/share/gems/gems/rake-13.0.3/exe/rake:27:in `<top (required)>'
Tasks: TOP => webpack:compile
(See full trace by running task with --trace)
error: Bad exit status from /var/tmp/rpm-tmp.0uyi44 (%build)
    Bad exit status from /var/tmp/rpm-tmp.0uyi44 (%build)

@ehelms
Copy link
Member

ehelms commented Oct 23, 2024

This change will require a specfile update (new dependency), and thus I don't think the packit build step will ever pass on the PR.

@MariaAga
Copy link
Member Author

@ehelms doesnt the packaging update (was merged already) take care of that?

@ehelms
Copy link
Member

ehelms commented Oct 23, 2024

@ehelms doesnt the packaging update (was merged already) take care of that?

The packaging PR (https://github.com/theforeman/foreman-packaging/pull/11048/files) adds the package but does not update Foreman to require the package. Thus it does not get pulled in during build time and the above error occurs. This is a bit of a chicken and egg problem. We do not update the spec with new dependencies until the code has the dependency merged, but RPM builds will fail until we update the spec with the new build. We tend to just accept that packit will fail in this case and that's OK.

@ekohl
Copy link
Member

ekohl commented Oct 23, 2024

The packaging PR (https://github.com/theforeman/foreman-packaging/pull/11048/files) adds the package but does not update Foreman to require the package. Thus it does not get pulled in during build time and the above error occurs. This is a bit of a chicken and egg problem. We do not update the spec with new dependencies until the code has the dependency merged, but RPM builds will fail until we update the spec with the new build. We tend to just accept that packit will fail in this case and that's OK.

theforeman/foreman-packaging#8414 intends to solve that by dynamically generating the build requires. We're working on dropping EL8, which was a blocker to using this. I also need to write the NodeJS side of it, but I don't expect to wrap that up in time for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Legacy JS PRs making changes in the legacy Javascript stack UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants