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

ProjectRequestLimit plugin: ignore projects in terminating state #8400

Merged
merged 1 commit into from
Apr 8, 2016

Conversation

csrwng
Copy link
Contributor

@csrwng csrwng commented Apr 7, 2016

Fixes BZ 1324465

@csrwng
Copy link
Contributor Author

csrwng commented Apr 7, 2016

@deads2k @derekwaynecarr

@deads2k
Copy link
Contributor

deads2k commented Apr 7, 2016

lgtm, but I'm inclined to hold this for 1.2.x.

@csrwng do you think its a severe enough problem to warrant inclusion?

@csrwng
Copy link
Contributor Author

csrwng commented Apr 7, 2016

IMHO it's not serious enough to be a blocker, but I'll defer to @abhgupta who owns this area
https://bugzilla.redhat.com/show_bug.cgi?id=1324465

@derekwaynecarr
Copy link
Member

QE should comment if they want to re-use the same name in their tests (which this wont fix) or if they just want to create a new project with different name.

@csrwng
Copy link
Contributor Author

csrwng commented Apr 7, 2016

Creating a project with a different name won't help... it's just a count of projects that you've requested.

@derekwaynecarr
Copy link
Member

I understand that. But the original source BZ was commenting on not being able to create a project again when the policy did not give access, and was not specific on if the new project shared the same name if I recall.

@abhgupta
Copy link
Member

abhgupta commented Apr 7, 2016

I had a quick offline discussion with @csrwng on this and it seems that the risk for this change is minimal and the benefit to the user experience is potentially big. The case is that the user is deleting their project and then trying to create another one within minutes. This could be when the user has run into some issues with their existing project and have resorted to deleting the project and starting afresh using some template. If they are blocked from creating a new project at this point, it just adds to their frustration and makes for a bad user experience.

My vote would be for getting this one in.

@csrwng
Copy link
Contributor Author

csrwng commented Apr 7, 2016

@smarterclayton ^^

@smarterclayton
Copy link
Contributor

So I can do the following:

  1. Create project
  2. Delete project
  3. Goto 1

in rapid succession, and cause the server to blow up with projects, thus defeating the point of projects? You should probably allow only 1 or 2 terminating projects to count.

@abhgupta
Copy link
Member

abhgupta commented Apr 7, 2016

@smarterclayton That's a good point - makes sense to restrict the number of projects in terminating state. However, mid-long term, wouldn't it also make sense to provide visibility via the console and the CLI to the user for their projects in terminating state? Probably just the project name/state without the ability to drill into the resource details would be good.

@liggitt
Copy link
Contributor

liggitt commented Apr 7, 2016

the projects list acl should not use anything other than policy... I don't want to special case "access to the project OR your username in an annotation"

@abhgupta
Copy link
Member

abhgupta commented Apr 7, 2016

@liggitt Agreed - the role binding might be deleted.

@csrwng csrwng force-pushed the ignore_terminating_projects branch from c4fbceb to e85d94e Compare April 7, 2016 19:10
@csrwng csrwng force-pushed the ignore_terminating_projects branch from e85d94e to 5ecea6d Compare April 7, 2016 19:12
@csrwng
Copy link
Contributor Author

csrwng commented Apr 7, 2016

@smarterclayton thx, updated to only allow a maximum of 2 terminating projects to not count towards the total considered for admission.

@smarterclayton
Copy link
Contributor

We could move role deletion to the very end of namespace deletion
@derekwaynecarr.

I think we should only allow 1 terminating project for now in this (make it
a variable, but doesn't have to be configurable unless we can come up with
a reason).

On Thu, Apr 7, 2016 at 3:08 PM, Abhishek Gupta [email protected]
wrote:

@liggitt https://github.com/liggitt Agreed - the role binding might be
deleted.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8400 (comment)

@smarterclayton
Copy link
Contributor

2 is fine

@csrwng
Copy link
Contributor Author

csrwng commented Apr 7, 2016

@deads2k still lgty ?

@deads2k
Copy link
Contributor

deads2k commented Apr 7, 2016

@deads2k still lgty ?

Yes, still lgtm.

If @smarterclayton approves. Seems like that @abhgupta will get to pick it up in a point release even if it doesn't make 1.2.

@smarterclayton
Copy link
Contributor

smarterclayton commented Apr 7, 2016 via email

@abhgupta
Copy link
Member

abhgupta commented Apr 7, 2016

@deads2k @csrwng I am hoping this can be merged and be included in 3.2

@abhgupta
Copy link
Member

abhgupta commented Apr 7, 2016

Ignore my comment above - clayton beat me to the comment by just 1 second.

@csrwng
Copy link
Contributor Author

csrwng commented Apr 7, 2016

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5543/) (Image: devenv-rhel7_3932)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 5ecea6d

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 5ecea6d

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2804/)

@openshift-bot openshift-bot merged commit 6ecbaaf into openshift:master Apr 8, 2016
@csrwng csrwng deleted the ignore_terminating_projects branch July 19, 2016 15:34
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.

7 participants