Fix to parse containerImage to get version #403

f-higashi · 2016-02-19T12:09:13Z

When I use private.registry:5000/nginx for container image, version label is set '5000/nginx' and it has error.
Fixed it.

codecov-io · 2016-02-19T12:45:26Z

Current coverage is `81.76%`

Merging #403 into master will increase coverage by +0.14% as of fd4a7d2

@@            master    #403   diff @@
======================================
  Files           75      77     +2
  Stmts          615     625    +10
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit            502     511     +9
  Partial          0       0       
- Missed         113     114     +1

Review entire Coverage Diff as of fd4a7d2

Powered by Codecov. Updated on successful CI builds.

bryk · 2016-02-19T12:48:29Z

src/app/frontend/deploy/deployfromsettings_controller.js

@@ -347,10 +347,19 @@ export default class DeployFromSettingsController {
   */
  getContainerImageVersion_() {
    /** @type {number} */
-    let index = (this.containerImage || '').lastIndexOf(':');
+    let index = (this.containerImage || '').lastIndexOf('/');


You know what...? I think we'll never be fully correct when doing manual URI parsing. Can we maybe use library https://github.com/derek-watson/jsUri to do the URI parsing for us? You can include the library in bower and use it straight away.

Or am I wrong? Can you convince me and other code readers that this algorithm works?

Second test actually shows that even if version is provided in the link it will be parsed as empty string, so potential version information is lost. Moreover checking only for : or / or some other char will be measured only for certains cases and not fully generic.

This library is a good idea and I'd go with that.

In our case there is special semantic attached to the URI. From my experience, parsers are often designed for simple http urls and are buggy for other use cases. But, we should give it a try and use this library. In any case I would be most convinced by explicit test cases. The tests in this PR are very nice, but not comprehensive to cover every possible URI

Yes, can we have more test cases for this feature?

f-higashi · 2016-02-19T22:38:44Z

Okay,
I'll try this library and add more test cases.

bryk · 2016-02-22T07:23:09Z

Awesome. Just comment when you're ready for rereview.

f-higashi · 2016-02-23T07:37:27Z

I checked jsUri and docker command.
But I think my idea is still simpler and easier to understand because using jsUri seems to cause more complicated condition.

'Container image' can be set with the following value:
[REGISTRY_HOST[:REGISTRY_PORT]/]NAME[:TAG]
cf) https://github.com/docker/docker/blob/master/docs/reference/commandline/pull.md

REGISTRY can be omitted and NAME can contain '/' as delimiters for namespace.

ex)

'Container image' contains registry url, namespace and tag
registry:5000/namespace/test:1
a result of jsUri is:

  host: registry
  port: 5000
  path: namespace/test:1

We can get 1 by parsing path
2. 'Container image' contains namespace and tag but does not contain registry url
namespace/test:1

host: namespace
path: test:1

We can get 1 by parsing path. But host property is not corrected.
It misleads developers.
3. 'Container image' contains tag but does not contain registry and namespace
test:1

host: test
port: 1
path: (empty)

In this case, we need to use existing way.

TAG cannot contain ':' and '/'. So it is no problem to get last '/'.

cf) https://github.com/docker/docker/blob/master/image/spec/v1.md

What do you think?

cheld · 2016-02-23T08:08:37Z

I would suggest to move the uri-logic to its own class, something like dockeruri.js Add meaningful methods in context of docker. If you use a library for parsing is an implementation detail, then.

bryk · 2016-02-23T08:20:09Z

Yeah, I like what Christoph said. Move the parsing logic to a separate file and do not use the library. In your code add implementation comments that explain why the code is correct (i.e., the way you explained this to us now). Add a few more tests and let's submit this.

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

f-higashi · 2016-02-24T05:20:58Z

PTAL

I moved logic to separate file.
A format of Image name is not URI. I cannot find the name in docker documents.
So I named DockerImageReference.

If you have suitable name, please tell me.

f-higashi · 2016-02-24T06:16:14Z

In my environment, 'gulp format' changes many files. (Now I modified format manually..)

For example)

--- a/src/app/frontend/common/filters/filters_module.js
+++ b/src/app/frontend/common/filters/filters_module.js
@@ -20,12 +20,11 @@ import relativeTimeFilter from './relativetime_filter';
 /**
  * Module containing common filters for the application.
  */
-export default angular
-    .module(
-        'kubernetesDashboard.common.filters',
-        [
-          'ngMaterial',
-        ])
+export default angular.module(
+                          'kubernetesDashboard.common.filters',
+                          [
+                            'ngMaterial',
+                          ])
     .filter('middleEllipsis', middleEllipsisFilter)
     .filter('kdMemory', memoryFilter)
     .filter('kdCores', coresFilter)

It seems to be affected by other setting.

My environment:

14.04.1-Ubuntu

Where should I check?
Do you have any idea?

bryk · 2016-02-24T10:45:12Z

There may be a new formatter release. If yes, I'll quickly fix this.

Review status: 0 of 4 files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

f-higashi · 2016-02-24T10:46:29Z

The above problem was solved.
After deleting files which is written in .gitignore, it works.

PTAL

bryk · 2016-02-24T10:57:05Z

A few style comments and let's merge :)

Review status: 0 of 4 files reviewed at latest revision, 5 unresolved discussions.

src/app/frontend/common/docker/dockerimagereference.js, line 22 [r4] (raw file):
Maybe delete this line? I cannot understand it.

src/app/frontend/common/docker/dockerimagereference.js, line 32 [r4] (raw file):
Should this have default value? I think not - it is not needed :)

src/app/frontend/common/docker/dockerimagereference.js, line 38 [r4] (raw file):
Typos: "If TAG is not omitted"

src/test/frontend/common/docker/dockerimagereference_test.js, line 17 [r4] (raw file):
How about "testImage:" case? What will it return? And ":"? And "::"? And "/"?

src/test/frontend/common/docker/dockerimagereference_test.js, line 65 [r4] (raw file):
This is repeated test. Identical as previous.

Comments from the review on Reviewable.io

f-higashi · 2016-02-24T13:34:57Z

I fixed it.

PTAL

bryk · 2016-02-25T08:26:46Z

Last comment PTAL. Then we can merge :)

Reviewed 1 of 2 files at r1, 1 of 3 files at r2, 1 of 2 files at r5.
Review status: 3 of 4 files reviewed at latest revision, 1 unresolved discussion.

src/app/frontend/common/docker/dockerimagereference.js, line 32 [r4] (raw file):
You need to add @type information to the this.identifier field. It should also be private (should end with trailing _). Like this.

/**
 * @private {string}
 */
this.identifier_ = identifier;

PTAL :)

Comments from the review on Reviewable.io

f-higashi · 2016-02-25T09:05:28Z

I added @type information.
And identifier is old name. I use reference in test code.
But I missed to change some part. Fix it too.

Review status: 3 of 4 files reviewed at latest revision, 1 unresolved discussion.

src/app/frontend/common/docker/dockerimagereference.js, line 32 [r4] (raw file):
I added @type information.
And identifier is old name. I use reference in test code.
But I missed to change some part. Fix it too.

Comments from the review on Reviewable.io

f-higashi · 2016-02-25T10:06:17Z

I found one issue.
Newer docker reference can has digest information.

[REGISTRY_HOST[:REGISTRY_PORT]/]NAME[:TAG][@DIGEST]

Is the issue fixed in this pull request?
Or this pull request merge and create new issue?

What do you think?

bryk · 2016-02-25T14:55:36Z

Is the issue fixed in this pull request?
Or this pull request merge and create new issue?
What do you think?

I say merge this and solve the problem in a new pull request.

bryk · 2016-02-25T14:56:24Z

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

bryk · 2016-02-25T14:56:33Z

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io

…image Fix to parse containerImage to get version

Fix to parse containerImage to get version

589bd53

googlebot added the cla: yes label Feb 19, 2016

Format file

91fbf42

bryk reviewed Feb 19, 2016
View reviewed changes

Move logic to separete file

2fe9504

f-higashi added 2 commits February 24, 2016 14:33

Fix missing semicolon and error of 'gulp check'

5685b8d

Fix missing semicolon

1514646

Fix format

98878ea

bryk mentioned this pull request Feb 24, 2016

Validate container image name on deploy page #422

Closed

f-higashi added 2 commits February 24, 2016 21:52

Remove default value from constructor

c164870

Add some test cases

9395a3c

Add @type information and fix name

9de1ccc

bryk added a commit that referenced this pull request Feb 25, 2016

Merge pull request #403 from f-higashi/fix-get-version-from-container…

cffd34a

…image Fix to parse containerImage to get version

bryk merged commit cffd34a into kubernetes:master Feb 25, 2016

f-higashi deleted the fix-get-version-from-containerimage branch March 3, 2016 05:23

anvithks added a commit to anvithks/k8s-dashboard that referenced this pull request Sep 27, 2021

Added register storage feature. (kubernetes#403)

f76f967

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to parse containerImage to get version #403

Fix to parse containerImage to get version #403

f-higashi commented Feb 19, 2016

codecov-io commented Feb 19, 2016

bryk Feb 19, 2016

bryk Feb 19, 2016

floreks Feb 19, 2016

cheld Feb 19, 2016

bryk Feb 19, 2016

f-higashi commented Feb 19, 2016

bryk commented Feb 22, 2016

f-higashi commented Feb 23, 2016

cheld commented Feb 23, 2016

bryk commented Feb 23, 2016

f-higashi commented Feb 24, 2016

f-higashi commented Feb 24, 2016

bryk commented Feb 24, 2016

f-higashi commented Feb 24, 2016

bryk commented Feb 24, 2016

f-higashi commented Feb 24, 2016

bryk commented Feb 25, 2016

f-higashi commented Feb 25, 2016

f-higashi commented Feb 25, 2016

bryk commented Feb 25, 2016

bryk commented Feb 25, 2016

bryk commented Feb 25, 2016

Fix to parse containerImage to get version #403

Fix to parse containerImage to get version #403

Conversation

f-higashi commented Feb 19, 2016

codecov-io commented Feb 19, 2016

Current coverage is 81.76%

bryk Feb 19, 2016

Choose a reason for hiding this comment

bryk Feb 19, 2016

Choose a reason for hiding this comment

floreks Feb 19, 2016

Choose a reason for hiding this comment

cheld Feb 19, 2016

Choose a reason for hiding this comment

bryk Feb 19, 2016

Choose a reason for hiding this comment

f-higashi commented Feb 19, 2016

bryk commented Feb 22, 2016

f-higashi commented Feb 23, 2016

cheld commented Feb 23, 2016

bryk commented Feb 23, 2016

f-higashi commented Feb 24, 2016

f-higashi commented Feb 24, 2016

bryk commented Feb 24, 2016

f-higashi commented Feb 24, 2016

bryk commented Feb 24, 2016

f-higashi commented Feb 24, 2016

bryk commented Feb 25, 2016

f-higashi commented Feb 25, 2016

f-higashi commented Feb 25, 2016

bryk commented Feb 25, 2016

bryk commented Feb 25, 2016

bryk commented Feb 25, 2016

Current coverage is `81.76%`