-
Notifications
You must be signed in to change notification settings - Fork 4.8k
new project #999
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
new project #999
Conversation
a8e47d1 to
a67a7a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use kubernetes/api/errors.IsNotFound()?
|
comments addressed and added some clean up if adding the admin use failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on further reflection, I don't think we should default the displayName to projectName either (at least not client-side). That defaulting should happen server-side if we want it. The UI falls back to the projectName if displayName is empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defaulting removed.
dd0052c to
b1160b3
Compare
|
@liggitt All defaulting removed, comment restored, and replaced cleanup with a message about adding a user to a role. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want a TODO to remove this once master policy namespace goes away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
07c82d2 to
7850d2c
Compare
|
rebased and unnested |
7850d2c to
3fe2aec
Compare
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1079/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits:
- put success first, then failure
- sub in the specified role rather than hardcoding admin
- put sample command on it's own line to make it easier to to copy/paste
fmt.Printf("The project %v was created, but %v could not be added to the %v role.\n", ...)
fmt.Printf("To add the user to the existing project, run\n\n\topenshift ex policy add-user --namespace=%v --role-namespace=%v %v %v\n", ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
3fe2aec to
72b4ec1
Compare
|
LGTM, squash and merge |
72b4ec1 to
4258b75
Compare
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/927/) (Image: devenv-fedora_805) |
86247ab to
4258b75
Compare
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin up to 4258b75 |
Merged by openshift-bot
|
The sample-app README needs to be updated to reflect this command being present. |
waiting on #991
Creates
openshift ex project new-project <projectname> --admin=anypassword:adminuser@liggitt