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

Extended.[images] openshift limit range admission should deny a push of built image exceeding limit on openshift.io/images resource #10375

Closed
bparees opened this issue Aug 12, 2016 · 12 comments
Assignees
Labels
component/imageregistry kind/bug Categorizes issue or PR as related to a bug. priority/P2

Comments

@bparees
Copy link
Contributor

bparees commented Aug 12, 2016

[Fail] [images] openshift limit range admission [It] should deny a push of built image exceeding openshift.io/Image limit 
/data/src/github.com/openshift/origin/test/extended/images/helper.go:148

[Fail] [images] openshift limit range admission [It] should deny a push of built image exceeding limit on openshift.io/images resource 
/data/src/github.com/openshift/origin/test/extended/images/helper.go:148

https://ci.openshift.redhat.com/jenkins/job/origin_extended_image_tests/395/consoleFull

Please fix or disable this test asap, it's getting run in the image bucket and causing that bucket to report failure.

@bparees bparees added kind/bug Categorizes issue or PR as related to a bug. priority/P2 component/imageregistry labels Aug 12, 2016
@bparees
Copy link
Contributor Author

bparees commented Aug 12, 2016

@liggitt @deads2k

@bparees
Copy link
Contributor Author

bparees commented Aug 12, 2016

(also ideally please change the tag on the test. we are using [images] for tests related to specific images (eg testing the ruby image), this sounds like a registry test to me)

@bparees
Copy link
Contributor Author

bparees commented Aug 16, 2016

@PI-Victor as the extended test babysitter this sprint, I suggest you create a PR to move this test (and any other related tests that aren't actually docker image tests) into their own bucket (eg [imageapis]). I've already discussed with @deads2k though i'm not sure i convinced him of the merits of that solution. You can also just convince @mfojtik instead which might be easier.

@mfojtik
Copy link
Contributor

mfojtik commented Aug 16, 2016

@bparees lol :-)

@deads2k
Copy link
Contributor

deads2k commented Aug 16, 2016

I've already discussed with @deads2k though i'm not sure i convinced him of the merits of that solution. You can also just convince @mfojtik instead which might be easier.

Not convinced enough enough to do it, but I don't feel strongly enough to argue either. Laying claim to someone else's API group for unrelated tests doesn't seem particularly polite.

@bparees
Copy link
Contributor Author

bparees commented Aug 16, 2016

Laying claim to someone else's API group for unrelated tests doesn't seem particularly polite.

@deads2k

  1. I guarantee we were using that tag before these tests came along because we were the first ones doing extended tests
  2. the tag in this case is based on the extended test directory name (running [images] runs all the tests that live under the test/extended/images) not api naming. The question would be why limitrange_admission was put in that directory, since it bears no relation to the other tests in that folder.
  3. The point here is not laying claim to particular names, it's ensuring separate things are in separate buckets so we can easily test specific areas of function and not pick up unrelated stuff, particularly when that stuff is not passing. If you want to make the case that all the existing image tests should move to a different bucket instead, you can make that argument (i'll fight you, at a minimum based on (1), but you can make it).

In sum, i don't buy the impoliteness argument. Also I did initially say "fix or disable this test" before proposing moving it to another bucket (which is still the best answer because we need an easy way to run buckets of tests and this isn't related to the existing bucket), but since no one did that for 4 days and I don't want our tests to continue failing and wonder if we we are ignoring deeper problems any longer, getting it out of the bucket is the most expedient action at this point.

@bparees
Copy link
Contributor Author

bparees commented Aug 16, 2016

And if i seem overly defensive of the images bucket here, it's because we've spent a huge amount of effort getting that bucket to a passing state and every time we do, someone does something that breaks it and makes it useless to us again.

@mfojtik
Copy link
Contributor

mfojtik commented Aug 22, 2016

@bparees @PI-Victor is this issue fixed now when we moved the tests?

@bparees
Copy link
Contributor Author

bparees commented Aug 22, 2016

yup

@bparees bparees closed this as completed Aug 22, 2016
@bparees
Copy link
Contributor Author

bparees commented Aug 22, 2016

well
the test is still failing.

@bparees bparees reopened this Aug 22, 2016
@bparees
Copy link
Contributor Author

bparees commented Aug 22, 2016

so it's not my problem anymore, but you should still fix the tests (or the bug that's causing them to fail).

@bparees bparees assigned mfojtik and bparees and unassigned PI-Victor and bparees Aug 22, 2016
miminar pushed a commit to miminar/origin that referenced this issue Sep 9, 2016
Resolves openshift#10375

Signed-off-by: Michal Minář <[email protected]>
@miminar
Copy link

miminar commented Sep 9, 2016

Here's a fix: #10835

miminar pushed a commit to miminar/origin that referenced this issue Sep 22, 2016
Resolves openshift#10375

Signed-off-by: Michal Minář <[email protected]>
miminar pushed a commit to miminar/origin that referenced this issue Sep 22, 2016
Resolves openshift#10375

Signed-off-by: Michal Minář <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/imageregistry kind/bug Categorizes issue or PR as related to a bug. priority/P2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants