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

Assume valid when name validation errors. #393

Merged
merged 1 commit into from
Mar 16, 2016

Conversation

bryk
Copy link
Contributor

@bryk bryk commented Feb 18, 2016

Addresses #373

Review on Reviewable

@bryk
Copy link
Contributor Author

bryk commented Feb 18, 2016

@codegoblins

@bryk
Copy link
Contributor Author

bryk commented Feb 18, 2016

@codegoblins can you review?

@codecov-io
Copy link

Current coverage is 80.58%

Merging #393 into master will not affect coverage as of cb64c1e

@@            master    #393   diff @@
======================================
  Files           75      75       
  Stmts          613     613       
  Branches         0       0       
  Methods          0       0       
======================================
  Hit            494     494       
  Partial          0       0       
  Missed         119     119       

Review entire Coverage Diff as of cb64c1e

Powered by Codecov. Updated on successful CI builds.

@codegoblins
Copy link

With this patch I was able to create a service & RC from the deploy flow called 'runtest':

screen shot 2016-02-18 at 1 28 14 pm

However, I am unable to do anything else from the dashboard e.g. edit pod count, view details:
screen shot 2016-02-18 at 1 30 15 pm

It seems like kubernetes/kubernetes/21464 is the real culprit here. If we want to fix this from the frontend side, we're just pushing the problem further down the deployment/administration flow.

@bryk
Copy link
Contributor Author

bryk commented Feb 19, 2016

However, I am unable to do anything else from the dashboard e.g. edit pod count, view details:

True. You'll be able to access it when running the Dashboard inside a cluster.

@bryk
Copy link
Contributor Author

bryk commented Feb 19, 2016

It seems like kubernetes/kubernetes/21464 is the real culprit here. If we want to fix this from the frontend side, we're just pushing the problem further down the deployment/administration flow.

You're correct. The linked issue is the real problem. This PR, however, addresses problem with our UI where we assume that name is invalid when we cannot determine its correctness. I believe we should assume it is valid because potential problems will be caught later.

@codegoblins
Copy link

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


Comments from the review on Reviewable.io

@codegoblins
Copy link

Confirmed that the problem is nonexistant when running from within the cluster.

@maciaszczykm
Copy link
Member

Should it be merged?

@bryk
Copy link
Contributor Author

bryk commented Mar 16, 2016

I say yes :) Merging.

bryk added a commit that referenced this pull request Mar 16, 2016
Assume valid when name validation errors.
@bryk bryk merged commit 571b0a2 into kubernetes:master Mar 16, 2016
@bryk bryk deleted the run-fails-as-app-name branch March 16, 2016 09:43
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.

5 participants