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

Various fixes and updates... #1

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

Various fixes and updates... #1

wants to merge 3 commits into from

Conversation

rm-rf-etc
Copy link

@rm-rf-etc rm-rf-etc commented Jul 17, 2018

What I've Done

I will add code comments for all of the changes which are more localized.

  • All uses of var were replaced with appropriate const or let.
  • All dynamic strings were updated to use string interpolation.

Outstanding Issues

Not all issues are fixed by this PR. server.js currently references two files which don't seem to exist in the container and I don't know what the fix is yet. These are the offending references:

server.js Line 69: /etc/scripts/code.sh
server.js Line 75: /etc/tcpdump -i veth0

Additionally, /etc/startup.sh gives me errors:

OCI runtime exec failed: exec failed: container_linux.go:348: starting container process caused "exec format error": unknown

I haven't investigated this error yet. I think it would help to know the original intent behind code.sh and tcpdump, as mentioned.

What Next

I'd like to get this project fully working before focusing on improvements. I think a react frontend would be a significant improvement over jquery, and wouldn't need to over-complicate the project. This is something I'd like to do, both for my own uses and to help others grok the app.

I don't yet have enough familiarity with TRex – can it simulate traffic through authenticated endpoints? Meaning, register -> login -> browse. Could we add an interface for generating traffic of this sort? Is this already easy to do?

Should this project instead by using https://hub.docker.com/r/trexcisco/trex/?

I'd like to add tests.

Thanks for constructing this example.

@@ -9,7 +9,7 @@
"express": "^4.16.3",
"jquery-min": "0.0.2",
"log-timestamp": "^0.1.2",
"pty": "0.0.0",
"node-pty": "^0.7.6",
Copy link
Author

Choose a reason for hiding this comment

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

I don't know which version of node you're running, but it appears that pty.js is not updated for more recent versions. I'm running v10.0.0. It appears that node-pty is a suitable update so I've made the change (this fixed problems).

const path = require('path');

const Docker = require('dockerode');
const docker = new Docker();
Copy link
Author

@rm-rf-etc rm-rf-etc Jul 17, 2018

Choose a reason for hiding this comment

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

Removed unused imports, swap pty.js for node-pty.


// Creating an HTTP server
var server = http.createServer(app).listen(8080)
const server = http.createServer(app).listen(7171);
Copy link
Author

@rm-rf-etc rm-rf-etc Jul 17, 2018

Choose a reason for hiding this comment

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

I changed port 8080 to 7171 simply because 8080 is used in many other projects, so this new port will hopefully avoid collisions with other projects the user wants to run on localhost (this happened to me, I typically will run a database API or the frontend of the app I'm building, on 8080, which is the thing I want to target with fake traffic).


```
Node.js
Docker
$ docker pull trexcisco/trex-dev:2.36
Copy link
Author

Choose a reason for hiding this comment

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

People may not realize they need to pull the docker image, and specifically trexcisco/trex-dev:2.36 in this case.

});
}


function create_socket() {
// Connect to the socket.io server
client.socket = io.connect('http://csi-trex-10:8080');
client.socket = io.connect('http://localhost:7171');
Copy link
Author

Choose a reason for hiding this comment

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

124: I assume you are using csi-trex-10 for localhost, this will trip some people up. Could alternatively put 127.0.0.1 here.

}

function on_window_resize() {
cols, rows = get_console_size();
const { cols, rows } = get_console_size();
Copy link
Author

Choose a reason for hiding this comment

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

Seems curly braces for destructuring are missing here. Also const is needed to prevent these from being assigned globally.


</body>
<!-- <body onresize="on_window_resize()"></body> -->
<body></body>
Copy link
Author

Choose a reason for hiding this comment

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

(I only tested in Chrome)

The on_window_resize process was already broken, but after fixing it, the experience in the frontend is actually worse, so I've commented out the resize function until I understand what the original intent was. Commenting out this resize function makes the frontend more usable.

For me, on_window_resize causes all content in the consoles to be reduced to just one character.

cols: cols,
rows: rows,
cols,
rows,
Copy link
Author

Choose a reason for hiding this comment

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

Might as well use short-hand syntax for these object properties.

cols: cols,
rows: rows,
cols,
rows,
Copy link
Author

Choose a reason for hiding this comment

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

Might as well use short-hand syntax for these object properties.

cols: dim.cols,
rows: dim.rows,
cols,
rows,
Copy link
Author

Choose a reason for hiding this comment

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

Might as well use short-hand syntax for these object properties.

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.

1 participant