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.

Sign up for GitHub

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

Jump to bottom

Drop From field validation check #1464

Merged
merged 1 commit into from
Mar 26, 2015
Merged

Drop From field validation check #1464

merged 1 commit into from
Mar 26, 2015

Conversation

0xmichalis
Copy link
Contributor

No description provided.

mfojtik
mfojtik reviewed Mar 26, 2015
View reviewed changes
pkg/build/api/validation/validation.go
@@ -239,7 +240,8 @@ func validateImageChange(imageChange *buildapi.ImageChangeTrigger) errs.Validati
if len(imageChange.Image) == 0 {
allErrs = append(allErrs, errs.NewFieldRequired("image"))
}
if len(imageChange.From.Name) == 0 {
var emptyObject kapi.ObjectReference
if imageChange.From == emptyObject {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kapi.ObjectReference should be pointer and you should just check for nil here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you but it's not a pointer :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mfojtik I have changed it to a pointer though (you wouldn't guess :P)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mfojtik I have changed it back to the original fix which I find more sane until someone decides that we have to change object references to pointers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bparees is there a reason why From is not (*kapi.ObjectReference) in types.go ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what's preferable here: &imageChange.From == nil or what I currently have in my PR ie. comparing against an empty object?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was wrong with the initial if len(imageChange.From.Name) == 0 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soltysh @Kargakis so in that case you need to check if the len(From.Name)>0 and the Kind (if set) is "imageRepository"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soltysh we check for the From.Name twice (see following else if check). We should check for From and From.Name separately.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If from won't be set it'll be empty by default, so you need to check just the name, IMHO.

@0xmichalis
Copy link
Contributor Author

@soltysh @mfojtik updated to check just the Name field.

@soltysh
Copy link
Member

soltysh commented Mar 26, 2015

LGTM

@0xmichalis 0xmichalis changed the title Validate From field Drop From field validation check Mar 26, 2015
@smarterclayton
Copy link
Contributor

We didn't have an tests of this? We could use test... :)

@0xmichalis
Copy link
Contributor Author

Added tests for validateImageChange

bparees
bparees reviewed Mar 26, 2015
View reviewed changes
pkg/build/api/validation/validation_test.go
name: "Both missing refs",
ict: &buildapi.ImageChangeTrigger{
Image: "",
From: kapi.ObjectReference{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about a test where you don't even define the From field? Just in case someone changes it to a ptr in the future and sets us up for a nil ptr error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But even if they change it to a pointer, they would have to turn into a pointer every from field in the previous tests too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup. i'm just paranoid and it seemed like an easy add. thanks for doing it! :)

@Kargakis
Drop From field validation check
d08fed8 
Currently if From.Name is zero the validation wrongly reports that the From field is required. Drop the From field validation check as it's not so useful.
@bparees
Copy link
Contributor

bparees commented Mar 26, 2015

lgtm. [merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1307/) (Image: devenv-fedora_1140)

@openshift-bot
Copy link
Contributor

Evaluated for origin up to d08fed8

openshift-bot pushed a commit that referenced this pull request Mar 26, 2015
Merge pull request #1464 from kargakis/ict-validation-fix
6d56cdb 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit 6d56cdb into openshift:master Mar 26, 2015
@0xmichalis 0xmichalis deleted the ict-validation-fix branch March 26, 2015 15:14
jpeeler added a commit to jpeeler/origin that referenced this pull request Oct 24, 2017
@jpeeler
Squashed 'cmd/service-catalog/go/src/github.com/kubernetes-incubator/…
ee4ff3f 
…service-catalog/' changes from aa27078754..510060232e

510060232e origin build: add origin tooling
de45e94 v0.1.0 chart changes (openshift#1468)
0bb9982 Modify Makefile to only specify ldflags once (openshift#1471)
5d6afac Fixes openshift#735: Add repo-sync script for charts (openshift#1453)
630f13f fix lingering unversioned client API (openshift#1466)
6f49128 Fix several logging errors (openshift#1464)
2aece61 Delete removed serviceClasses when they have no instances left (openshift#1450)
179d302 Uncommenting UID field after updating to k8s 1.8 (openshift#1457)
b70c076 Reorder class and plan creation; test plan conflict handling (openshift#1459)
4bea012 Use versioned client APIs (openshift#1458)
ff4af30 clean up logic for 410 gone deprovision poll (openshift#1452)
3fddf27 clean up logic and fix message for failed poll (openshift#1451)
40926cd Fix typo from openshift#1354 (openshift#1456)
ff86ef2 Delete removed serviceplans when they have no instances left (openshift#1444)
8411a16 tweak binding setAndUpdateOrphanMitigation function (openshift#1448)
ce28252 Combine apiserver and controller-manager into a single service-catalog image (openshift#1343)
7bbc8ee Check service class / plan before allowing provisioning or plan changes. (openshift#1439)
baf28de Create listers before adding event handlers in controller (openshift#1446)
294157d remove setServiceBindingCondition dependency on controller (openshift#1441)
118a0f7 Fix typo in validation (openshift#1447)
117bfbd clean up error logging (openshift#1443)
dff470f Move "External" around in some resource names/properties (openshift#1354)
0885edb Adding expectedGot function and using it. (openshift#1440)
a7d582e Pretty controller broker (openshift#1442)
c5edfaf Set apimachinery build variables with semver info (openshift#1429)
0e90d82 Add a pretty formatter for ClusterService[Class|Plan] (openshift#1408)
fb874df Remove deprecated basic auth config support (openshift#1431)
f4cd181 Migrate to metav1 methods for manipulating controllerRefs (openshift#1433)
96b286e Make service/plan reference fields on instance spec selectable (openshift#1422)
33f2b04 First example using the pretty context builder. (openshift#1403)
7852917 Stop using corev1.ObjectReference and corev1.LocalObjectReference (openshift#1417)
fcf9480 Add tests for plan updates (openshift#1412)
819332e Add root CAs (openshift#1419)
b49a76a Clean Makefile a little (openshift#1399)
d681da0 Use a separate etcd prefix for each integration test to keep tests isolated (openshift#1415)
314a622 Wire etcd prefix to storage and call complete with options (openshift#1394)
REVERT: aa27078754 origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: 510060232e54eb64b294213bb5d7847e169a2fac
jpeeler added a commit to jpeeler/origin that referenced this pull request Feb 1, 2018
@jpeeler @kibbles-n-bytes
Fix several logging errors (openshift#1464)
6f49128 
There were several problems with incorrect number of arguments, unnecessary
quoting, and not using httpErr's error method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@0xmichalis @soltysh @smarterclayton @bparees @openshift-bot @mfojtik @Kargakis