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

docker: validation for run image #1772

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@dperpeet
Copy link
Member

commented Feb 4, 2015

Fixes #1708
screenshot from 2015-02-04 12 24 45

Placement of warnings/errors needs design @andreasn

@dperpeet dperpeet added the needsdesign label Feb 4, 2015

@andreasn andreasn self-assigned this Feb 5, 2015

@petervo

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2015

Seems like a good aproach, we may want to break validation up so that all fields aren't validated every time.

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2015

All the ports need to be looked at in their entirety - otherwise we can't skip empty lines or detect duplicate entries.

If we don't look at everything, we have to have states to keep track of errors so we know when to disable the 'run' button.

Given the low number of fields and the time between checks (not after each keydown) I'm not sure it's worth splitting up the checks.

@petervo

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2015

I agree that all ports should validate together, but when you think about adding volumes and links there could be quite a bit more to run through so we may want to split things up a little. Instead of storing error state you can trigger updating the state of the button based on the presence of a "error" class.

Anyways if you are really oppsed to breaking it up that's fine we can refactor if it becomes a problem.

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2015

Using the presence of an error class is a good idea, that would work nicely.

I will refactor the pull request once linking containers is in.

@dperpeet dperpeet force-pushed the dperpeet:docker_validation branch from 06536b0 to 33787f4 Feb 9, 2015

@andreasn

This comment has been minimized.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2015

Good start!
Even though it's nice to have the error as close to the input form as possible, it breaks the layout, so I think it's better to put it below. This is what Patternfly recommends.

Mockup

@andreasn andreasn removed the needsdesign label Feb 9, 2015

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2015

Updated design @andreasn
screenshot from 2015-02-09 15 49 56

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2015

updated design, also for links @andreasn
screenshot from 2015-02-09 22 39 58

@dperpeet dperpeet changed the title WIP docker: validation for run image docker: validation for run image Feb 9, 2015

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2015

@andreasn should the select somehow be marked when there is a warning? It is currently unaffected by has-warning/has-error tags.

@dperpeet dperpeet force-pushed the dperpeet:docker_validation branch 2 times, most recently from d1bd1b0 to a6e3bee Feb 9, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2015

Seems like there's an integration test regression here?

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2015

@stefwalter I will look into why the test passes on my system and fails on hubbot...

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2015

@andreasn if docker links are specified incompletely (only alias or only container), should there be an error instead of a warning?

@dperpeet dperpeet force-pushed the dperpeet:docker_validation branch from a6e3bee to c3fabb0 Feb 10, 2015

@mvollmer

This comment has been minimized.

Copy link
Member

commented Feb 10, 2015

Seems like there's an integration test regression here?

Looks like a spurious failure. Running just check-docker succeeds.

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2015

I removed whitespace from the testcase strings, that should reduce such spurious failures in the future.

@mvollmer

This comment has been minimized.

Copy link
Member

commented Feb 10, 2015

I removed whitespace from the testcase strings, that should reduce such spurious failures in the future.

Hmm, is there evidence that the funny whitespaces used by term.js are to blame? It's a pretty obscure thhing that one needs to keep in mind when modifying the tests, but it shouldn't have an effect on reliability. Does it?

(We could make writing the tests easier by providing a term_to_string function or similar that converts non-breaking whitespace to normal whitespace. What do you think?)

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2015

Hmm, is there evidence that the funny whitespaces used by term.js are to blame? It's a pretty obscure thhing that one needs to keep in mind when modifying the tests, but it shouldn't have an effect on reliability. Does it?

One of the lines that failed was the very first, simple check for the container output. Removing whitespace was the most pragmatic way I could think of to resolve this issue for now.

Converting whitespace would work. At the moment I don't believe it's worth the effort, though.

@dperpeet dperpeet removed the needswork label Feb 11, 2015

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2015

should the select somehow be marked when there is a warning? It is currently unaffected by has-warning/has-error tags.

@dperpeet yes, I think that would make sense. Easiest is probably to make the text red. We can add some extra styling for now, and then I'll file a issue upstream in patternfly.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2015

if docker links are specified incompletely (only alias or only container), should there be an error instead of a warning?

Sure.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2015

Getting golden color for warnings are kind of ugly, but that seems to be what we get from patternfly for now. Will file an issue about that.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2015

Filed the color issue as patternfly/patternfly#49

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2015

We should never show errors when the user has not yet touched the dialog. Perhaps this point is missing from our dialog HIG?

I'm not sure I agree. When I open the dialog and it's clear that the current configuration isn't viable - even better, I know why and can pinpoint the place - why not alert the user?

Otherwise this gets insane with reflows. In addition the port/link boxes that now have pretty strange margins when errors are shown etc.

Obviously all of the above issues can be fixed given enough effort, but it may unrealistic to do them for all the dialogs across Cockpit. Perhaps we should set our sights a bit lower with error display, by highlighting invalid fields, and displaying a relevant error message near the dialog buttons?

I completely agree regarding the reflow issues.

@andreasn: We thought we could model this like the change password dialog. That dialog is a lot simpler, hence the immediate checks. I propose that we remove all on-input and keypress validation when the dialog starts. Once the user presses 'run' and we detect errors, those checks become enabled to let the user take care of the errors without hitting run again.

This way, an experienced person will never see errors for running a working configuration, no matter if any intermediate states were erroneous. Some who needs help will profit from errors disappearing as soon as an issue was resolved.

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2015

I'm not sure I agree. When I open the dialog and it's clear that the current configuration isn't viable - even better, I know why and can pinpoint the place - why not alert the user?

Because it's a bad user experience. You end up castigating the user who hasn't even done anything yet. We talked about this at length last year. But we can have this discussion again. It seems like Yin failed to note this in the HIG, but it was an important point of getting our validation worked out.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2015

In addition the port/link boxes that now have pretty strange margins when errors are shown etc.

This is a styling error of those boxes and I filed #1811 about that.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2015

There is a small glitch in the error validation if you jump from one dialog box to another too quickly.

@mvollmer

This comment has been minimized.

Copy link
Member

commented Feb 16, 2015

One of the lines that failed was the very first, simple check for the container output. Removing
whitespace was the most pragmatic way I could think of to resolve this issue for now.

I am still not convinced that the whitespace has anything to do with the reliability of check-docker. What about splitting the commit docker: remove whitespace from messages in tests to reduce string for… out from this pull request?

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2015

I am still not convinced that the whitespace has anything to do with the reliability of check-docker. What about splitting the commit docker: remove whitespace from messages in tests to reduce string for… out from this pull request?

Whitespace can lead to problems regarding encoding, so removing whitespace will increase the reliability of check-docker. One could argue that this hack only hides an underlying issue... I will split the commit as you suggested.

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2015

In addition to the latest proposed changes (no error checking until user presses run for the first time) we also decided to remove the warnings - they are more clutter than they help.

@dperpeet dperpeet force-pushed the dperpeet:docker_validation branch from ed9b1f6 to 8f333bd Feb 16, 2015

@dperpeet dperpeet removed the needsdesign label Feb 16, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2015

Hmmm, there seems to be a remaining issue:

  • I can add invalid ports (containing text), and the dialog will try and run the command, leading to a failure later from Docker.

@dperpeet dperpeet force-pushed the dperpeet:docker_validation branch from 8f333bd to 0d9cc58 Feb 17, 2015

@dperpeet dperpeet removed the needswork label Feb 17, 2015

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2015

I can add invalid ports (containing text), and the dialog will try and run the command, leading to a failure later from Docker.

Added check

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2015

Duplicate ports check didn't work for me (like if another container is already using port 3000), so I get a error dialog instead.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2015

Everything works really neat! Great job on this one!

@dperpeet dperpeet added the needswork label Feb 17, 2015

@dperpeet dperpeet force-pushed the dperpeet:docker_validation branch from 0d9cc58 to 1eff27d Feb 17, 2015

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2015

@andreasn please check again, I forgot to push changes proposed by @stefwalter (use hidden instead of display: none)
also I fixed #1825

@dperpeet dperpeet removed the needswork label Feb 17, 2015

@dperpeet dperpeet force-pushed the dperpeet:docker_validation branch from 1eff27d to a9941e3 Feb 18, 2015

@dperpeet dperpeet force-pushed the dperpeet:docker_validation branch from a9941e3 to 8f15ca2 Feb 18, 2015

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2015

updated checks

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2015

I've tested this, and all seems to work!
Probably smaller kinks to fix here and there, but good to go from a UI point of view.

@dperpeet dperpeet closed this in 6b747f5 Feb 18, 2015

@dperpeet dperpeet deleted the dperpeet:docker_validation branch Mar 18, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.