Implement listing images/containers using React JSX #3

QiWang19 · 2018-07-26T18:57:54Z

QiWang19 · 2018-07-26T18:59:12Z

martinpitt

Great work already! I reviewed roughly a third of this PR, but a lot of comments apply to several places in the code. Also, this needs to grow integration tests along with the code, to ensure it is (and stays) working (also with newer podman versions).

martinpitt · 2018-07-26T20:11:20Z

src/ContainerDetails.js

+class ContainerDetails extends React.Component {
+    constructor(props) {
+        super(props);
+    }


This component has no state, so it should be a functional component. (another interesting blog post). Also, it's custom to explicitly spell out the properties that you support/expect, like this:

const ContainerDetails({container}) => ( <div> ... <dt>{ container.Created.. }</dt> </div> );

martinpitt · 2018-07-26T20:22:57Z

package.json

+    "bootstrap-less": "^3.3.8",
+    "create-react-class": "^15.6.3",
+    "exenv": "^1.2.2",
+    "jquery": "^3.3.1",


Please avoid introducing jQuery in new code, and particularly not with react. I don't see it actually being used here? I think this can just be dropped. Likewise, please comb the list for other unused stuff, like "exenv" -- it just unnecessarily enlarges npm install time, and all of these dependencies introduce maintenance. react-create-class also sounds like dead weight -- use proper ES6 classes instead.

I changed it. But dockerUI uses Id.

martinpitt · 2018-07-26T20:24:08Z

src/ContainerDetails.js

+        return (
+            <div className='listing-ct-body'>
+                <dl>
+                    <dt>{_("Id")}      </dt> <dd>{ container.ID }</dd>


Please avoid the extra whitespace in the element. Perhaps they are not rendered, but it might still cause some layout issues. Also please be consistent with putting the <dd>s into a new line.

"Id" should be spelled "ID", as it's an acronym.

martinpitt · 2018-07-26T20:24:52Z

src/Containers.js

+class Containers extends React.Component {
+    constructor(props) {
+	super(props);
+	console.log(props.containers);


Debugging leftover.

martinpitt · 2018-07-26T20:26:28Z

src/Containers.js

+	if (state.Status)
+	    return state.Status;
+	if (state.Running)
+	    return "running";


Needs i18n (_("") wrapper)

martinpitt · 2018-07-26T20:33:41Z

src/Containers.js

+    }
+
+    render() {
+	console.log(this.props.onlyShowRunning);


debugging leftover

martinpitt · 2018-07-26T20:34:11Z

src/Containers.js

+	//TODO
+	let emptyCaption = _("No containers");
+
+	let filtered = this.props.containers.filter((container)=>{


Missing spaces around operator, and unnecessary braces around a single argument.

martinpitt · 2018-07-26T20:35:20Z

src/Containers.js

+	let filtered = this.props.containers.filter((container)=>{
+	    if (this.props.onlyShowRunning && !container.State.Running)
+		return false;
+	    return true;


write this more succinctly as `filter(container => (!this.props.onlyShowRunning || container.State.Running));

martinpitt · 2018-07-26T20:36:59Z

src/ContainersRunImageModal.js

+    constructor(props) {
+      super(props);
+
+    }


Unnecessary if you don't do anything other than calling the parent. Also, this component can be functional, it doesn't have any state.

martinpitt · 2018-07-26T20:37:17Z

src/ContainersRunImageModal.js

+      return (
+	<Modal
+	  isOpen={this.props.show}
+	>


IMHO this looks really ugly - just write it as one line, it's short enough.

QiWang19 · 2018-07-30T14:02:04Z

I fixed the review issues you found. @martinpitt PTAL

martinpitt

Thanks, nice progress! Still a bunch of small issues, and two large ones:

This still does not update the tests, they fail now. It's acceptable to not cover all the new functionality with tests right away, this can happen in follow-up PRs (but must happen nevertheless). But they at least must succeed.
Please use consistent indentation. 8 spaces seems rather excessive and makes a lot of code rather "narrow". We use 4 everywhere else.

martinpitt · 2018-08-01T10:05:26Z

src/index.html

    <link rel="stylesheet" href="../base1/patternfly.css">
+    <link href="docker.css" type="text/css" rel="stylesheet" id="term-style">


Please rename this to podman.css (same for the file) to avoid confusion.

martinpitt · 2018-08-01T10:09:16Z

src/Containers.js

+		"",
+		// TODO: MemoryUsage, MemoryLimit
+		"",
+		state,


This should still be i18n'ed, but I'm happy for this to be a // TODO: i18n comment for now.

martinpitt · 2018-08-01T10:11:47Z

src/Containers.js

+	let columns = [
+		{ name: container.Name, header: true },
+		image,
+		container.Config.Entrypoint === "" ? container.Config.Cmd.join(" ") : container.Config.Cmd.join(""),


The second join is still confusing, please add a comment what it does.

martinpitt · 2018-08-01T10:14:35Z

webpack.config.js

+            {
+                test: /\.es6$/,
+                loader: "babel-loader"
+            },


These all already exist above and are duplicated.

martinpitt · 2018-08-01T10:16:03Z

src/ContainerDetails.js

@@ -0,0 +1,33 @@
+import React from 'react';


This file must be named .jsx, to apply correct babel and eslint rules. It's not "classic" non-ES6 JavaScript. Same applies to the other *.js files in this PR.

martinpitt · 2018-08-01T10:19:11Z

webpack.config.js

+            {
+                test: /views\/[^\/]+\.html$/,
+                loader: "ng-cache?prefix=[dir]"
+            },


This looks unrelated and should be a separate commit with a proper justification.

martinpitt · 2018-08-01T10:19:41Z

webpack.config.js

+            },
+            {
+                test: /[\/]angular\.js$/,
+                loader: "exports?angular"
            }
        ]


There's no angular in this code, and you should stay really far away from introducing it :-) Please drop this.

martinpitt · 2018-08-01T10:21:21Z

src/ContainerHeader.js

+import cockpit from 'cockpit';
+import Select from '../lib/cockpit-components-select.jsx';
+
+const _ = cockpit.gettext;cockpit.gett


There's some junk here

martinpitt · 2018-08-01T10:22:34Z

src/ContainerHeader.js

+            filterText: ''
+        };
+
+        this.filterChanged = this.filterChanged.bind(this);


This doesn't seem necessary, you only call this from a method that already has a correct this.

martinpitt · 2018-08-01T10:23:32Z

src/ContainerHeader.js

+
+    handleFilterChange (value) {
+        this.setState({ filter: value });
+        this.props.onChange(value);


Should check if the onChange property is actually defined. Or declare it a mandatory property.

QiWang19 · 2018-08-02T17:32:22Z

@martinpitt I have made the changes you requested. Sorry, I am still getting used to the github workflow.

martinpitt

Some more issues inline, also you still didn't update the tests. Please make sure that make check works.

But this PR really isn't working like that, so some things about development and review in general:

Don't add more and more new features (like the vuln scanning) to this single PR. It is already immensely large and really difficult to review, and at this rate it gets worse faster than it gets better.
You should do lots of small PRs, each of which implement one self-contained new feature or bug fix, together with comprehensive integration tests. A small PR is much easier and faster to review and understand, both by you and the reviewer.

For example, you could do one PR to move the varlink interface into a new utils.es6, one PR to add a container list, one PR to add an image list, etc.

Finish one PR and get it landed before working on new stuff. If you don't finish the old one, nothing will land at the end.

This isn't meant to be offensive or discouraging, I just want to guide you how you can actually land changes effectively.

Thanks! FYI, I'll be on PTO the next two weeks, so please ask in #cockpit for reviewers.

martinpitt · 2018-08-03T10:39:19Z

src/Containers.jsx

+            container.Config.Entrypoint === "" ? container.Config.Cmd.join(" ") : container.Config.Cmd.join(""),
+            container.State.Running ? utils.format_cpu_percent(container.HostConfig.CpuPercent) : "",
+            containerStats ? utils.format_memory_and_limit(containerStats.mem_usage, containerStats.mem_limit) : "",
+            state,


As I said, please add a TODO: i18n comment here.

martinpitt · 2018-08-03T10:41:41Z

src/Containers.jsx

+        let columns = [
+            { name: container.Name, header: true },
+            image,
+            //Concat the cmd directly if has entrypoint, otherwise join cmd with space.


This doesn't really explain anything, it's just translating JS to English. It's quite clear what the code does. It's also clear why we want to join the container entrypoint command array with spaces. But it needs to be explained why the config command does not get spaces without an entry point. It's still a comand array, isn't it? How does that not look broken?

martinpitt · 2018-08-03T10:44:25Z

webpack.config.js

+            {
+                test: /\.jsx$/,
+                loader: "babel-loader"
+            },


This is still duplicated from above. You shouldn't/can't have to loaders for .js and .jsx. You need to merge the strict and babel loader into one list, if you really need both.

martinpitt · 2018-08-03T10:46:27Z

webpack.config.js

+            {
+                test: /\.less$/,
+                loader: extract.extract("css-loader?sourceMap&minimize=!less-loader?sourceMap&compress=false")
+            },


Why do you need the css and less handlers? For some node dependencies? That usually isn't required in your own project (but I could miss something there)

Because I used cockpit-components-listing.jsx from docker UI, it import the less

martinpitt · 2018-08-03T10:47:52Z

src/ContainerHeader.jsx

+            filter: 'running',
+            filterText: ''
+        };
+        this.filterChanged = this.filterChanged.bind(this);


Already from the previous review, but it now got hidden; This doesn't seem necessary, you only call this from a method that already has a correct this.

martinpitt · 2018-08-03T10:50:40Z

src/Images.jsx

+		this.handleCancelImageDeleteModal = this.handleCancelImageDeleteModal.bind(this);
+		this.handleRemoveImage = this.handleRemoveImage.bind(this);
+		this.handleCancelImageRemoveError = this.handleCancelImageRemoveError.bind(this);
+		this.handleForceRemoveImage = this.handleForceRemoveImage.bind(this);


Please don't just wholesale bind all of your methods. This is only required if you use them as handler attributes in a HTML tag, like <button onClick={this.myHandler}. As soon as you call it from one of the class'es methods or even from an arrow method (<button onClick={() => this.myHandler(...)) you don't need this bind. E. g. at least renderRow() and navigateToImage() are unnecessary binds.

(This is a nitpicking of course, and doesn't really hurt - but it makes the code longer, and it's good to understand what this actually does).

martinpitt · 2018-08-03T10:52:30Z

src/Images.jsx

+	}
+
+	showRunImageDialog(e) {
+		e.preventDefault()


preventDefault is a rather large axe, especially if it's unconditional for all events. This always needs a comment that justifies why it's necessary.

martinpitt · 2018-08-03T10:53:54Z

src/app.jsx

@@ -21,110 +21,149 @@
 import cockpit from 'cockpit';
 import React from 'react';
 import './app.scss';


Either drop src/app.scss and this import, or move your podman.scss definitions in there. Don't keep two, that's confusing.

martinpitt · 2018-08-03T10:55:18Z

src/index.html

@@ -23,6 +23,7 @@
    <meta name="viewport" content="width=device-width, initial-scale=1">

    <link rel="stylesheet" href="../base1/patternfly.css">
+    <link href="podman.css" type="text/css" rel="stylesheet" id="term-style">


Please don't do this. Nothing in the .html file itself should need the CSS, only the App JSX object. Import it in src/app.jsx like it currently imports app.scss (see above, there should only be one file).

QiWang19 · 2018-08-08T18:14:51Z

@martinpitt @garrett @andreasn @stefwalter @larskarlitski I've added some tests. PTAL.

garrett · 2018-08-09T07:30:30Z

@QiWang19: Hey! I saw your request in IRC. I'm replying here to let you know you were heard. 😉

If you have design questions or some HTML and/or CSS questions, I'm happy to help. I even know JavaScript pretty OK. However, I don't know React/JSX... outside of modifying it to make HTML/CSS/JS a bit better.

As your PR looks like a lot of JSX, I probably can't help at this time in this particular PR.

That all said, I've been a fan of Podman (and everything related, like Buildah) for a while and I'm really looking forward to having Podman support in Cockpit!

martinpitt · 2018-08-20T06:29:38Z

Back from PTO, having a look. Tests on master started failing with

Failed to enable unit: Unit file io.projectatomic.podman.socket does not exist.

as podman apparently renamed the socket. This will also break this PR.

martinpitt

After PR #4 to fix the socket path this now needs a rebase, and applying the socket renaming to your branch. I also found a few small issues which I am fixing myself.

The test also fails:

  File "test/check-application", line 40, in testBasic
    b.wait_visible('#containers-containers tr:contains("busybox:latest")')
  File "test/common/testlib.py", line 285, in wait_visible
    return self.wait_js_func('ph_is_visible', selector)
  File "test/common/testlib.py", line 270, in wait_js_func
    self.wait_js_cond("%s(%s)" % (func, ','.join(map(jsquote, args))))
  File "test/common/testlib.py", line 267, in wait_js_cond
    self.raise_cdp_exception("timeout\nwait_js_cond", cond, result["exceptionDetails"], trailer)
  File "test/common/testlib.py", line 167, in raise_cdp_exception
    raise Error("%s(%s): %s" % (func, arg, msg))
testlib.Error: timeout
wait_js_cond(ph_is_visible("#containers-containers tr:contains(\"busybox:latest\")")): #containers-containers tr:contains("busybox:latest") not found

I am going to investigate this, and then force-push your branch, to unblock this.

martinpitt · 2018-08-20T06:39:58Z

.gitignore

@@ -13,3 +13,5 @@ test/common/
 test/images/
 *.pot
 POTFILES*
+tmp/*
+/src/test.jsx


This looks like a debugging leftover. Don't add such local files to .gitignore, you are going to forget about them.

martinpitt · 2018-08-20T06:48:15Z

test/check-application

        # create a container
-        m.execute("podman run -d --name swamped-crate busybox sleep 1000")
+        # m.execute("podman run -d --name swamped-crate alpine sleep 10000")


Leftover stuff, should be dropped.

martinpitt · 2018-08-20T06:49:14Z

test/check-application


-        # FIXME: UI needs to listen to change signals and update automatically
        b.reload()


This reload should now be dropped as well.

martinpitt · 2018-08-20T06:49:35Z

test/vm.install

@@ -15,3 +15,4 @@ fi
 # grab a few images to play with; tests run offline, so they cannot download images
 podman pull busybox
 podman pull docker.io/alpine
+# podman rm eager_murdock


martinpitt · 2018-08-20T07:02:18Z

test/check-application

+        b.wait_visible('#containers-images tbody tr:contains("busybox:latest") + tr button.btn-delete')
+
+        # show all containers to interact with stopped ones
+        m.execute("podman run -d --name swamped-crate busybox sh")


This cannot work, the container will exit immediately. This is why in my initial tests I ran sleep 10000 in it.

Oh, I see, it's supposed to be in the "exited" state, as you switch to "everything".

Closes cockpit-project#3

martinpitt · 2018-08-20T07:20:16Z

I did an initial force push to fix the conflict and make the test actually work. Note that I had to put back the b.reload(), as the UI does not react to container changes. But I'm going to push again, there are tons of tabs and wrong indentations in your .jsx files (JS uses 4 space indentation, and no tabs). There are also still some earlier review comments which still apply.

Closes cockpit-project#3

martinpitt

I did another force push to fix the tab damage, and a few other minor issues. Let's land this now, to unblock this.

Please do a follow-up PR with completing the tests. For example:

Start a running container and make sure that it appears so.
Check that container names and container commands are shown correctly. Particularly with commands that have more than one word, to cover correct spacing.
Cover the "Commit", "Delete", and "Start" buttons in container details. The latter currently causes a crash, this needs to be fixed as well.
Image details and deletion.

martinpitt · 2018-08-20T07:41:23Z

Tests succeeded, landing. Thanks!

QiWang19 mentioned this pull request Jul 26, 2018

This can realize listing images and containers using React and JSX. Please review. #2

Closed

martinpitt requested changes Jul 26, 2018

View reviewed changes

QiWang19 force-pushed the images branch from ac6b0a6 to c5c4682 Compare July 30, 2018 13:59

martinpitt requested changes Aug 1, 2018

View reviewed changes

QiWang19 force-pushed the images branch 2 times, most recently from f3c2e83 to d529ae8 Compare August 2, 2018 17:30

QiWang19 force-pushed the images branch from 8a8ba25 to fcca7e1 Compare August 2, 2018 18:37

martinpitt requested changes Aug 3, 2018

View reviewed changes

QiWang19 force-pushed the images branch 7 times, most recently from 47d8f40 to bb15d9c Compare August 8, 2018 16:04

martinpitt requested changes Aug 20, 2018

View reviewed changes

martinpitt reviewed Aug 20, 2018

View reviewed changes

martinpitt force-pushed the images branch from bb15d9c to 6677162 Compare August 20, 2018 07:16

martinpitt pushed a commit to QiWang19/cockpit-podman that referenced this pull request Aug 20, 2018

Implement listing images using React and JSX

6677162

Closes cockpit-project#3

Implement listing images using React and JSX

c147fe9

Closes cockpit-project#3

martinpitt force-pushed the images branch from 6677162 to c147fe9 Compare August 20, 2018 07:28

martinpitt approved these changes Aug 20, 2018

View reviewed changes

martinpitt merged commit d847a2f into cockpit-project:master Aug 20, 2018

This was referenced Aug 29, 2018

Add missing tests #6

Closed

Implement container stop, start, restart, delete. Add Manual testing for delete. #5

Closed

		<link rel="stylesheet" href="../base1/patternfly.css">
		<link href="docker.css" type="text/css" rel="stylesheet" id="term-style">


		# FIXME: UI needs to listen to change signals and update automatically
		b.reload()

Implement listing images/containers using React JSX #3

Implement listing images/containers using React JSX #3

Conversation

QiWang19 commented Jul 26, 2018

QiWang19 commented Jul 26, 2018

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QiWang19 commented Jul 30, 2018

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QiWang19 commented Aug 2, 2018

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QiWang19 commented Aug 8, 2018 • edited Loading

garrett commented Aug 9, 2018

martinpitt commented Aug 20, 2018

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt commented Aug 20, 2018

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt commented Aug 20, 2018

QiWang19 commented Aug 8, 2018 •

edited

Loading