-
Notifications
You must be signed in to change notification settings - Fork 492
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
base: master
Are you sure you want to change the base?
Conversation
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.
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be removed perhaps?
There was a problem hiding this comment.
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')){ |
There was a problem hiding this comment.
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')) {
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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.
webpack.config.js
Outdated
"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' |
There was a problem hiding this comment.
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" )
]
There was a problem hiding this comment.
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...
Could not get |
USER stf | ||
curl -sf https://gobinaries.com/tj/node-prune | sh | ||
|
||
wget --progress=dot:mega \ |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
- Working on a linux machine, fetching the PR:
$ git fetch origin pull/118/head:nanoscopic
$ git checkout nanoscopic
- 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)
- 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
- 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 theDockerfile-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 aDockerfile
.
- 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?
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: