-
Notifications
You must be signed in to change notification settings - Fork 41
Conversation
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.
LGTM, except for one comment left, and was just curious that why were tests failing initially when you sent this PR and what fixed that?
And great work! 🎉
glide.yaml
Outdated
- pkg/apis | ||
- package: k8s.io/apimachinery | ||
version: release-1.6 |
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.
Why did we pin the version to "release-1.6"? client-go v3.0.0 does that already, right? Any reason we had to specify that explicitly here?
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.
If I don't pin it, it's master was being pulled leading it to not compile.
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.
gotcha, can you add that as a comment?
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.
comment where?
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.
:) In glide.yaml, on the line above - package: k8s.io/apimachinery
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.
but comments in glide.yml are bit tricky as everytime this file gets modified using glide
command comments are lost :-( we must be careful with that
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.
haha, same time @kadel :P
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.
umm, @kadel glide update
will modify glide.lock, not glide.yml, right?
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, thanks @kadel @containscafeine
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.
umm, @kadel glide update will modify glide.lock, not glide.yml, right?
yes, only thing that automatically modifies glide.yaml
as far as I know is glide get
I will wait on @kadel to merge this one, cause this is big one! |
Also lock down k8s.io/apimachinery to release-1.6
@kadel is this good to go in? |
Haven't got time to look at it closely yet 😞 |
@@ -1,5 +1,5 @@ | |||
/* | |||
Copyright 2014 The Kubernetes Authors. | |||
Copyright 2017 The Kedge Authors All rights reserved. |
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 you rebase with current master?
This diff looks weird. single_container_object.go
file shouldn't even exist after #180 being merged
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.
I did somehow it came, but yeah updated and pushed!
Fixes #81