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

Updating gulp, webpack, and Dockerfile #118

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

nanoscopic
Copy link

The main purpose of all the contained changes is to bring STF into a maintainable state using a greatly updated build process.

Some deprecated modules have been updated and/or removed/replaced entirely with others.

This merge also contains a weird "click here to respond to IOS dialogs" button under the advanced section of the UI. That should really be removed before this is merged as it isn't relevant to Android, and it isn't even really needed due to clicking on such dialogs working fine in the latest code.

I would like feedback on:

  1. Any bits of the updated code that are ugly / perhaps might not belong
  2. Some testing of the code to ensure Android functionality still works properly after all these changes. I have only really thoroughly tested IOS support with the changes in this PR.

nanoscopic and others added 15 commits July 9, 2020 17:36
Unstable commit; committing to track and save progress.

Add Dockerfile to .dockerignore to prevent changes to the Dockfile itself from triggering rebuild of earlier steps of the Dockerfile

Move to Ubuntu 18 for Dockerfile instead of 16. Not yet moving to Ubuntu 20 as Node 8 is not supported on Ubuntu 20.

Rewrote Dockerfile as a multi-stage build to both prevent constant rebuild of everything on small changes and also to only copy needed files into the end runtime.

Moved a number of modules from bower.json to package.json as updated webpack was not able to recognize those specific modules and do dynamic module resolution correctly with them unless the npm version of those modules are used.

Moved to using a different lato font package as the specific module was/is not on npm.

Split webpack config up into two files as it was done in a non-standard looking way to begin with. Now the webpack config file contains only the standard looking webpack config as it should.

Updated gulp to a new version and rewrote the old incompatible bits of the gulpfile to work with the new version.

Temporarily removed deletion of /res/build from clean step of the gulpfile.

Corrected hardcoded module references to modules that are now in node_modules/@DeviceFarmer

Moved prepublish step of package.json into Dockerfile. This could confuse users not using docker though and may have to be changed back.

Updated to a new version of webpack and altered the webpack file to work with the new way. Angular is still broken and will need to be updated. The old angular method in webpack doesn't work with the new webpack version. TODO

Using versions of less-loader and sass-loader a major version behind as the latest versions require node 10.

Added '-loader' to all the various webpack loaders used not only in webpack config but also in a few module require lines.

Removed the commons.entry.js script lines as the new webpack build doesn't create a commons script.

Attempted to fix the way ngRoute/app is loaded by angular in auth/mock. Still isn't working. Am going to switch to newer angular...

Ran lang/translations json files through a JSON pretty printer as it looked horrible and lack of spaces is not user / curious person friendly.

webpack config file still in progress of changes.
Updated Dockerfile to use even more stages to separate frontend build from backend build.
The 'res' folder has mixed stuff in it for both the frontend and backend so this process is still not complete.

Moved all the angular modules into their own grouping in bower.json for readability. Previously attempted to move all the angular modules into npm package.json but that failed so they have to remain here in bower.json for now.

Updated angular to version 1.8.0. This works "for the most part" with various things broken here and there that am in process of fixing.

Added ng-annotate to webpack process, so as to use ngInject to get some dependencies running in places where old webpack 2.0 process was handling it. Updated a few places that need njInject annotation so far.

Added missing d3 import into app.js

Updated signinController to use `then` instead of deprecated ( and no longer working ) success and error functions.

Continued to update webpack config so that it has what is needed to startup the frontend using webpack 4. Login screen works now but is missing styling.

Main page after login is currently broken on angular-hotkeys and/or mousetrap. That is being worked on.
The updated build is nearly complete / fully functional. There are still some broken bits here and there but loading, styling, login, navigation are working correctly now.

Changed naming of bower installed module directory to 'bower_modules' and hardcoded it for the moment to /tmp/build rather than locating it within the res/ directory. Initially this change was made for testing purposes and to make the docker image build process easier/cleaner. Leaving it this way for the moment but it is problematic in that the /tmp directory is wiped on boot in some cases so if using in a non-dockerized way this is trouble. TODO: Change it back.

Added a bowerc file in initial attempts to use a proxy for fetching bower modules. Fetching bower modules through a proxy is nearly useless for what I intended ( caching ) because https is used. A MITM caching https proxy would need to be used. A caching proxy is not super important as long as you avoid running the docker build process hundreds of times in a row...

Changed back to using slightly older version of oboe. Why? The newer oboe 2.1.5 doesn't understand relative URLs; demanding all urls passed to it to start with http or https.

Added in spin.js dependency as it is needed and doesn't work without it. ( how did it work before? )

Using newly rewritten mousetrap module ( I basically rewrote it entirely as an ES6 module as the old version was a global and confusing and horrible and I disliked it very much )

Added in dependencies on epoch and ng-epoch. For whatever reason they weren't getting pulled in automatically.

Changed to using newer/updated angular-borderlayout2. This causes a 44 pixel blank space at the top of STF. Unsure quite sure how to get rid of it yet besides altering the css provided by the new module not to have that space. May fork the module temporarily to avoid this space in a clean way.

Added typescript and ts processing as some newer modules need it. Added babel and babel processing of jsx files to handle the es6 jsx within the mousetrap module, and to support future use and updates to the newer cleaner es6 class notation.

Module loading via the "require(x).name" method has been tweaked in a few places to detect when it fails to get a module name to pass to angular. Really the way this is done currently should be changed everywhere as it is hard to debug when modules don't work correctly and/or don't return a module name.

Epoch is now "epoch-charting". Changed some references to it. I think the old version may be getting pulled in also now? Need to check this further. TODO

Changes font loading to use esModule: false option so that all fonts get loaded from their font files as a URL fetch instead of whatever was happening before ( the before of which simply didn't work )

Fixed style loading ( was missing style-loader )
Use exact version of oboe. Use newer ng-table and new capital import name for NgTableParams.
Use angular-borderlayout2 instead of angular-borderlayout as it is updated more.
Fix hardcoded js links to angular-hotkeys to be correct to new non pre-built version.
Fix hardcoded ladda links.
Remove non-functioning transform code in webpack.
Reorder dependency resolve in webpack as the order matters due to equally named local modules.
Minor tweaks to Dockerfile. Addition of chown to /app/res as it was ending up owned by root instead of stf. Added webpackserver.config.js because it is looked at by the backend app in some cases.

Using latest mousetrap via angular-hotkeys.

Changed references to angular-borderlayout to all have it without a 2 at the end. Using custom fork as the original was broken with current browsers / node.

Added mime type setting to express; but it isn't needed. The MIME issue I was encountering was due to an error message being served out in place of a JS file, and that having MIME text/html. TODO: Remove this added line.

Added log of static file serving via express because there was no log anywhere of files being served and it made debugging difficult. TODO: Make this tied to a debug log level instead of always logging all the requests and cleanup the commented out bits.

Changed require('request') lines in various places to not use strict SSL, so that the self-signed / auto created certs of a demo / testing environment can actually work. This should be changed to use a custom single instance shared module that has configuration for specifying the root CA that created these certs, or at the very least making this options as it weakens security. TODO

Added logging of websocket requests to the frontend for debugging purposes. These can stay in place as they aren't visible unless someone goes looking for them in the console.
The advanced section has a new button now to show a dialog for responding to an on-screen alert.

Initial mouse wheel support for scrolling screen vertically has been added, but it has issues. If you scroll more than a single "notch" at a time it can freeze the WDA.

For now have added in console logging of messages via the websocket to the backend. This makes events far easier to debug and also generally see what is happening.
The wheel wire protocol was using an unsigned int but a signed in was being passed. That was causing huge values instead of small negatives
Bumps [then-jade](https://github.com/then/then-jade) from 2.4.3 to 2.4.4.
- [Release notes](https://github.com/then/then-jade/releases)
- [Commits](pugjs/then-pug@v2.4.3...v2.4.4)

Signed-off-by: dependabot-preview[bot] <[email protected]>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Bumps [imports-loader](https://github.com/webpack-contrib/imports-loader) from 0.6.5 to 0.8.0.
- [Release notes](https://github.com/webpack-contrib/imports-loader/releases)
- [Changelog](https://github.com/webpack-contrib/imports-loader/blob/master/CHANGELOG.md)
- [Commits](webpack-contrib/imports-loader@v0.6.5...v0.8.0)

Signed-off-by: dependabot-preview[bot] <[email protected]>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Bumps [rethinkdb](https://github.com/rethinkdb/rethinkdb) from 2.3.3 to 2.4.2.
- [Release notes](https://github.com/rethinkdb/rethinkdb/releases)
- [Commits](https://github.com/rethinkdb/rethinkdb/commits)

Signed-off-by: dependabot-preview[bot] <[email protected]>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Karol Wrótniak <[email protected]>
Merging upstream minus conflicting commit 841b092
This commit is essentially a conflict-resolved merge of DeviceFarmer@841b092

Previous upstream merge did not include this feature in order to merge more cleanly.
Copy link
Member

@koral-- koral-- left a comment

Choose a reason for hiding this comment

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

PR checks do not pass

var server = http.createServer(app)


/*app.use(function (req, res, next) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this comment?

Copy link
Author

Choose a reason for hiding this comment

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

No I will remove it. During change process I entirely broke css loading. I wrote the code block to help diagnose the process by which css is included. It has outlived its purpose as I figured it out and corrected it.

Be aware that I changed the font loading process. The old font loading process embedded the fonts into javascript code. The new process loads them via their files directly. I was unable to figure out how to get the old font process working with newer webpack. In the end I decided that it's okay to load font files directly.

require('ladda/dist/spin.min.js')
require('ladda/dist/ladda.min.js')
require('ladda/css/ladda.scss')
//require('spin.js/spin.ts')
Copy link
Member

Choose a reason for hiding this comment

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

should be removed perhaps?

Copy link
Author

Choose a reason for hiding this comment

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

Agreed. In my haste I've left commented out lines here and there. I will remove these. The way ladda works is rather convoluted and it took a few tries to figure out the combination that works with the newer version and updated loading process.

return ApkReader.open(file.path).then(function(reader) {
return reader.readManifest()
})
if(file.path.endsWith('.apk')){
Copy link
Member

Choose a reason for hiding this comment

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

Formatting seems to be inconsistent here. Taking into account other files it should be rather:
if (file.path.endsWith('.apk')) {

Copy link
Author

Choose a reason for hiding this comment

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

I'll change this. The style in the code is agreeably to have space between 'if and (', and this particular style difference is not something I care significantly about either way so I'm okay with sticking to the spaced style.

"registry": "https://registry.bower.io",
"directory": "res/bower_components"
"registry": "http://registry.bower.io",
"directory": "/tmp/build/bower_modules",

Choose a reason for hiding this comment

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

Even if there is an explanation in the commit message, I'd rather not keep it that way.

Comment on lines 33 to 39
"node_modules",
//path.resolve(__dirname, "bower_components"), // works
'/tmp/build/bower_components',
//path.resolve(__dirname, "res/web_modules" ), // works
'/tmp/build/res/web_modules',
//path.resolve(__dirname, "res/app/components" )// works
'/tmp/build/res/app/components'

Choose a reason for hiding this comment

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

I'd prefer something without absolute hardcoded path. Something like:

modules: [
      'node_modules',
      'bower_components',
      'web_modules',
      path.resolve(__dirname, "res/app/components" )
]

Copy link
Author

Choose a reason for hiding this comment

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

I agree that a relative path is needed. This is part of what is breaking stf local also I believe.

I spent far too long fighting with the pathing and order of precedence here and once I got it working I threw in the towel and stopped changing it.

As mentioned ultimately bower modules should go away entirely anyway at some point...

@pcrepieux
Copy link

Could not get stf local running properly after fixing (hopefully) the 'webpack'/'absolute path' issue.
I got the supposedly device screen layout on top of the controls panel and its content is empty...
Does it ring a bell ? Could this be related to the webpack stuff that I may have only partially managed on my side ?
(there are some independent changes that are mixed up making it harder to identify the root cause)

USER stf
curl -sf https://gobinaries.com/tj/node-prune | sh

wget --progress=dot:mega \

Choose a reason for hiding this comment

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

The wget command has to be prefixed by the RUN command, otherwise the build is failing at the beginning

"@devicefarmer/stf-syrup": "^1.0.1",
"@devicefarmer/stf-wiki": "^1.0.0",
"@julusian/jpeg-turbo": "^0.5.4",
"@slack/client": "^3.5.4",

Choose a reason for hiding this comment

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

@slack/client package added twice which cause the package.json file to be updated during installation

Copy link

@denis99999 denis99999 left a comment

Choose a reason for hiding this comment

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

@nanoscopic ,

I have tested your PR and it seems like there is almost nothing that is working well, here is a bulk of the steps I took along with the errors I encountered:

  1. Working on a linux machine, fetching the PR:
$ git fetch origin pull/118/head:nanoscopic
$ git checkout nanoscopic
  1. STF installation
$ rm -rf res/bower_components/* build/* node_modules/*
$ npm install
  • installation is OK, except that package.json file is updated to remove the @slack/client package added twice (cf. my previous review)
  • $ npm test failed :
> @devicefarmer/[email protected] test /home/debian/device-farmer/stf
> gulp test

[19:04:02] Using gulpfile ~/device-farmer/stf/gulpfile.js
[19:04:02] Starting 'test'...
[19:04:02] Starting 'lint'...
[19:04:02] Starting 'jsonlint'...
[19:04:02] 'jsonlint' errored after 27 ms
[19:04:02] Error: File not found with singular glob: /home/debian/device-farmer/stf/.yo-rc.json (if this was purposeful, use `allowEmpty` option)
    at Glob.<anonymous> (/home/debian/device-farmer/stf/node_modules/glob-stream/readable.js:84:17)
    at Object.onceWrapper (events.js:315:30)
    at emitOne (events.js:116:13)
    at Glob.emit (events.js:211:7)
    at Glob._finish (/home/debian/device-farmer/stf/node_modules/glob-stream/node_modules/glob/glob.js:197:8)
    at done (/home/debian/device-farmer/stf/node_modules/glob-stream/node_modules/glob/glob.js:182:14)
    at Glob._processSimple2 (/home/debian/device-farmer/stf/node_modules/glob-stream/node_modules/glob/glob.js:688:12)
    at /home/debian/device-farmer/stf/node_modules/glob-stream/node_modules/glob/glob.js:676:10
    at Glob._stat2 (/home/debian/device-farmer/stf/node_modules/glob-stream/node_modules/glob/glob.js:772:12)
    at lstatcb_ (/home/debian/device-farmer/stf/node_modules/glob-stream/node_modules/glob/glob.js:764:12)
    at RES (/home/debian/device-farmer/stf/node_modules/inflight/inflight.js:31:16)
    at f (/home/debian/device-farmer/stf/node_modules/once/once.js:25:25)
    at FSReqWrap.oncomplete (fs.js:152:21)
[19:04:02] 'lint' errored after 29 ms
[19:04:02] 'test' errored after 31 ms
npm ERR! Test failed.  See above for more details.
  • $ ./node_modules/gulp/bin/gulp.js eslint-cli failed :
[19:05:57] Using gulpfile ~/device-farmer/stf/gulpfile.js
[19:05:57] Starting 'eslint-cli'...
...
✖ 566 problems (300 errors, 266 warnings)

[19:06:03] 'eslint-cli' errored after 6.34 s
[19:06:03] Error in plugin 'eslint-cli'
Message:
    ESLint error
  • $ ./node_modules/gulp/bin/gulp.js build complete but with many errors :
[19:08:51] Using gulpfile ~/device-farmer/stf/gulpfile.js
[19:08:51] Starting 'build'...
[19:08:51] Starting 'clean'...
[19:08:51] clean
[19:08:51] Finished 'clean' after 17 ms
[19:08:51] Starting 'webpack:build'...
[19:08:51] webpack:build
2020-10-20T17:08:52.017Z INF/webpack:config 3142 [*] Build progress 0% (compiling)
2020-10-20T17:08:53.583Z INF/webpack:config 3142 [*] Build progress 14% (building)
2020-10-20T17:08:54.582Z INF/webpack:config 3142 [*] Build progress 18% (building)
2020-10-20T17:08:55.608Z INF/webpack:config 3142 [*] Build progress 26% (building)
2020-10-20T17:08:56.650Z INF/webpack:config 3142 [*] Build progress 30% (building)
2020-10-20T17:08:57.710Z INF/webpack:config 3142 [*] Build progress 40% (building)
2020-10-20T17:08:58.711Z INF/webpack:config 3142 [*] Build progress 50% (building)
2020-10-20T17:08:59.919Z INF/webpack:config 3142 [*] Build progress 67% (building)
2020-10-20T17:09:00.921Z INF/webpack:config 3142 [*] Build progress 69% (building)
[19:09:01] [webpack:build] Hash: cd38a028fb7b8924ef16
Version: webpack 4.44.2
Time: 9277ms
Built at: 2020-10-20 19:09:01
                               Asset      Size    Chunks                         Chunk Names
     0.cd38a028fb7b8924ef16.chunk.js   122 KiB         0  [emitted] [immutable]
     1.cd38a028fb7b8924ef16.chunk.js   564 KiB         1  [emitted] [immutable]
     2.cd38a028fb7b8924ef16.chunk.js   856 KiB         2  [emitted] [immutable]
     3.cd38a028fb7b8924ef16.chunk.js   715 KiB         3  [emitted] [immutable]
     4.cd38a028fb7b8924ef16.chunk.js  8.33 KiB         4  [emitted] [immutable]
     5.cd38a028fb7b8924ef16.chunk.js  8.45 KiB         5  [emitted] [immutable]
ecd86754157a1c3ab17926a520f52456.png  6.02 KiB            [emitted] [immutable]
                  entry/app.entry.js  10.8 KiB       app  [emitted]              app
             entry/authldap.entry.js  9.61 KiB  authldap  [emitted]              authldap
             entry/authmock.entry.js  9.61 KiB  authmock  [emitted]              authmock
f9be889d7634297548e90338cc603a97.png  5.71 KiB            [emitted] [immutable]
    fonts/lato-latin-100-italic.woff  21.4 KiB            [emitted]
   fonts/lato-latin-100-italic.woff2  16.6 KiB            [emitted]
    fonts/lato-latin-100-normal.woff  26.4 KiB            [emitted]
   fonts/lato-latin-100-normal.woff2  21.1 KiB            [emitted]
    fonts/lato-latin-300-italic.woff  21.9 KiB            [emitted]
   fonts/lato-latin-300-italic.woff2  17.2 KiB            [emitted]
    fonts/lato-latin-300-normal.woff  29.3 KiB            [emitted]
   fonts/lato-latin-300-normal.woff2  22.7 KiB            [emitted]
    fonts/lato-latin-400-italic.woff  29.1 KiB            [emitted]
   fonts/lato-latin-400-italic.woff2  23.9 KiB            [emitted]
    fonts/lato-latin-400-normal.woff    28 KiB            [emitted]
   fonts/lato-latin-400-normal.woff2  22.9 KiB            [emitted]
    fonts/lato-latin-700-italic.woff  29.2 KiB            [emitted]
   fonts/lato-latin-700-italic.woff2  23.9 KiB            [emitted]
    fonts/lato-latin-700-normal.woff  27.4 KiB            [emitted]
   fonts/lato-latin-700-normal.woff2  22.5 KiB            [emitted]
    fonts/lato-latin-900-italic.woff  28.3 KiB            [emitted]
   fonts/lato-latin-900-italic.woff2  23.1 KiB            [emitted]
    fonts/lato-latin-900-normal.woff  26.9 KiB            [emitted]
   fonts/lato-latin-900-normal.woff2    22 KiB            [emitted]
Entrypoint app = entry/app.entry.js
Entrypoint authldap = entry/authldap.entry.js
Entrypoint authmock = entry/authmock.entry.js
[./res/app/app.js] 1.1 KiB {app} [built]
[./res/app/components/stf/standalone/index.js] 630 bytes {2} [built]
[./res/app/control-panes/index.js] 1.52 KiB {2} [built]
[./res/app/device-list/index.js] 879 bytes {2} [built]
[./res/app/docs/index.js] 1.49 KiB {2} [built]
[./res/app/group-list/index.js] 1.09 KiB {2} [built]
[./res/app/layout/index.js] 1.17 KiB {2} [built]
[./res/app/menu/index.js] 641 bytes {2} [built]
[./res/app/settings/index.js] 676 bytes {2} [built]
[./res/app/user/index.js] 280 bytes {2} [built]
[./res/auth/ldap/scripts/entry.js] 502 bytes {authldap} [built]
[./res/auth/ldap/scripts/signin/index.js] 497 bytes {4} [built]
[./res/auth/mock/scripts/entry.js] 500 bytes {authmock} [built]
[./res/auth/mock/scripts/signin/index.js] 497 bytes {5} [built]
[./res/common/lang/index.js] 370 bytes {2} [built]
    + 587 hidden modules

ERROR in ./res/app/app.js
Module not found: Error: Can't resolve 'angular' in '/home/debian/device-farmer/stf/res/app'
 @ ./res/app/app.js 6:2-20

ERROR in ./res/auth/ldap/scripts/entry.js
Module not found: Error: Can't resolve 'angular' in '/home/debian/device-farmer/stf/res/auth/ldap/scripts'
 @ ./res/auth/ldap/scripts/entry.js 4:2-20

ERROR in ./res/auth/mock/scripts/entry.js
Module not found: Error: Can't resolve 'angular' in '/home/debian/device-farmer/stf/res/auth/mock/scripts'
 @ ./res/auth/mock/scripts/entry.js 4:2-20

ERROR in ./res/app/components/stf/common-ui/notifications/index.js
Module not found: Error: Can't resolve 'angular-animate' in '/home/debian/device-farmer/stf/res/app/components/stf/common-ui/notifications'
 @ ./res/app/components/stf/common-ui/notifications/index.js 1:0-26
 @ ./res/app/components/stf/common-ui/index.js
 @ ./res/app/layout/index.js
 @ ./res/app/app.js
...
...
[19:09:01] Finished 'webpack:build' after 9.57 s
[19:09:01] Finished 'build' after 9.59 s
2020-10-20T17:09:02.021Z INF/webpack:config 3142 [*] Build progress 100% (complete)
  1. Trying to launch STF using CLI in local mode (after launching of rethinkdb):
$ systemctl start rethinkdb-stf
$ ./bin/stf local --public-ip 192.168.1.13
  • No error at the Linux console, but on the client browser side, the authentication screen is not displayed because of this error:
Error: Cannot find module 'bootstrap/dist/css/bootstrap.css'
    webpackMissingModule http://192.168.1.13:7100/static/app/build/0.cd38a028fb7b8924ef16.chunk.js:2360
    js http://192.168.1.13:7100/static/app/build/0.cd38a028fb7b8924ef16.chunk.js:2360
    __webpack_require__ http://192.168.1.13:7100/static/app/build/entry/authmock.entry.js:64
    js http://192.168.1.13:7100/static/app/build/entry/authmock.entry.js:212
  1. Building a docker image from the Dockerfile (after fixing a little bug in that file, cf. my previous review)
$ docker build .
...
$ docker images
REPOSITORY                                                              TAG                                        IMAGE ID            CREATED             SIZE
<none>                                                                  <none>                                     48ecb446363d        36 seconds ago      1.27GB
<none>                                                                  <none>                                     4bdaa8dd0638        2 minutes ago       6.44MB
<none>                                                                  <none>                                     d7a29b34236d        2 minutes ago       1.62GB
<none>                                                                  <none>                                     35e1d2cc4794        2 minutes ago       1.78GB
...
$ docker tag 48ecb446363d devicefarmer/stf:nanoscopic
  • The build is OK, but the multi-stage building in your Dockerfile created three very huge additional layers, and I feel the main image is also very huge (1.27 GB) compared to an image created from the Dockerfile-debian-x86_64 (~600 MB), but it seems it was already the case before so an optimization stage could be made, in any case it is not really acceptable in particular for an ARM target as a Raspberry PI for instance and for which by the way you do not provide a Dockerfile.
  1. Trying to launch STF using the previous built container in local mode (after launching of rethinkdb)
$ systemctl start rethinkdb-stf
$ docker run --rm --net host devicefarmer/stf:nanoscopic stf local --public-ip 192.168.1.13
  • It failed and logs displayed that error:
    docker: Error response from daemon: OCI runtime create failed: container_linux.go:346: starting container process caused "exec: \"stf\": executable file not found in $PATH": unknown.

  • I also launched STF in full deployment mode and I encountered the same previous error for each unit

Conclusion

  • I feel this PR is not acceptable because nothing is working properly unless I am wrong it is possible !
  • I feel you should conform to the CONTRIBUTING.md file in order to produce a clean PR with a single initial commit after validating that it is working fine, that no regression is introduced (i.e. android features, eslint, npm test, etc.), that it has been tested in all environments (ie ARM / x86_64, Mozilla / Safari / Chrome / Edge, Linux / MacOS, etc.), with a separate documentation explaining functional changes or additions, like for example this bulky PR: Addition of a powerful system for device booking & partitioning openstf/stf#1056, because sorry to say that it is not really the role of a reviewer to do this work nor to evaluate as I did a PR in such a state, I think your work is still in the development stage and not in sufficient finished state to submit it as a PR
  • I feel that this preliminary work that you do for the reception of the iOS functionality, and given what you present, is very likely to introduce irreparable regressions into the product which has worked perfectly so far. In addition, the STF code is still very Android oriented at all levels (ie GUI, API, core) and I do not see how the introduction of iOS can be done by offering the same level of functionality everywhere, without at the same time refactoring all the code with the great risk of instability that this implies. So, I think it might be wise, rather than merging with the main branch of STF, that you create a new official branch or a new product under DeviceFarmer like for example STF-iOS or STF 2.0, in which you could lead all this upgrade work, without risk of degradation of the existing one, until the whole is judged sufficiently mature to allow us to move on to the new complete solution,
    What do you think about it?

@denis99999
Copy link

@nanoscopic ,

From now on with PR #500 STF is compatible with the latest version of node, and with PR #510 being merged we should be able to do without bower, so there are no more significant limits to evolve the STF code, therefore on your side would you be willing to cleanly add iOS functionality to STF?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants