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

Vendor registry packages differently to enable GCS #9211

Merged
merged 3 commits into from
Jun 9, 2016

Conversation

smarterclayton
Copy link
Contributor

Allows registry to have different packages than Kube.

@smarterclayton
Copy link
Contributor Author

[test] @mfojtik @pweil- @ncdc this will work in Go 1.6, but not in 1.4. For 1.4, it'll have to be a custom GOPATH.

I need to add the build logic to build this separately still.

@ncdc
Copy link
Contributor

ncdc commented Jun 8, 2016

😿😿😿 I think what we're having to do here deserves some crying cats 😿😿😿

@smarterclayton
Copy link
Contributor Author

Surprisingly, it's not as bad as I thought. With some scripting it would
probably be a trivial update.

On Jun 7, 2016, at 8:34 PM, Andy Goldstein [email protected] wrote:

😿😿😿 I think what we're having to do here deserves some crying cats 😿😿😿


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9211 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABG_pw5e4dirfR4E3rOh8Nsks42uwMXAks5qJg4UgaJpZM4Iwdmf
.

@ncdc
Copy link
Contributor

ncdc commented Jun 8, 2016

Sure, but it's ~25k LOC that we'd ideally not have to carry along

@smarterclayton
Copy link
Contributor Author

I'm starting to change my mind a bit here - I would rather use their
dependencies and have us change them willy nilly. I think we're going to
see this problem get worse (this is just the beginning).

On Jun 7, 2016, at 8:37 PM, Andy Goldstein [email protected] wrote:

Sure, but it's ~25k LOC that we'd ideally not have to carry along


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9211 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABG_pyu1o6Vl3pdydO5UYBb6jhAEUe9Dks5qJg6rgaJpZM4Iwdmf
.

@ncdc
Copy link
Contributor

ncdc commented Jun 8, 2016

We also discussed splitting the registry into a separate repo and having it vendor in all the deps it needs. Maybe we wouldn't have conflicting versions then, since what the registry would need to pull in from Kube and Origin should be relatively small. I'm not saying I want to go this route, but it's maybe starting to sound a bit better?

@smarterclayton
Copy link
Contributor Author

Today it would not be relatively small, but once we have a client library,
yes.

On Jun 7, 2016, at 8:46 PM, Andy Goldstein [email protected] wrote:

We also discussed splitting the registry into a separate repo and having it
vendor in all the deps it needs. Maybe we wouldn't have conflicting
versions then, since what the registry would need to pull in from Kube and
Origin should be relatively small. I'm not saying I want to go this route,
but it's maybe starting to sound a bit better?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9211 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABG_p3BYewuKnPvOKBFvDGNr9KzI8J07ks5qJhDdgaJpZM4Iwdmf
.

@smarterclayton smarterclayton changed the title WIP - Vendor registry packages differently Vendor registry packages differently to enable GCS Jun 8, 2016
@smarterclayton
Copy link
Contributor Author

Alright, this is now updated with build and test.

@smarterclayton
Copy link
Contributor Author

I cut the package imports down a bit and will push once I see a jenkins run.

@smarterclayton
Copy link
Contributor Author

[test]

@smarterclayton
Copy link
Contributor Author

[test]

@mfojtik
Copy link
Contributor

mfojtik commented Jun 8, 2016

@legionus FYI (this is not affecting you yet ;-)

@pweil-
Copy link

pweil- commented Jun 8, 2016

also, @miminar

@smarterclayton
Copy link
Contributor Author

When we land distribution 2.4 we'd need to do the same thing here, ping me
and I'll go over what is necessary.

On Wed, Jun 8, 2016 at 8:45 AM, Paul Weil [email protected] wrote:

also, @miminar https://github.com/miminar


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9211 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p_9xnXkQPzx3TP-nyq76g9Tqh-0Yks5qJrlQgaJpZM4Iwdmf
.

@smarterclayton
Copy link
Contributor Author

[test]

On Wed, Jun 8, 2016 at 10:24 AM, Clayton Coleman [email protected]
wrote:

When we land distribution 2.4 we'd need to do the same thing here, ping me
and I'll go over what is necessary.

On Wed, Jun 8, 2016 at 8:45 AM, Paul Weil [email protected]
wrote:

also, @miminar https://github.com/miminar


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9211 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p_9xnXkQPzx3TP-nyq76g9Tqh-0Yks5qJrlQgaJpZM4Iwdmf
.

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 005c45d

@openshift-bot
Copy link
Contributor

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

@smarterclayton
Copy link
Contributor Author

Ready for review - once we merge this I'll do a backport.

On Jun 8, 2016, at 5:50 PM, OpenShift Bot [email protected] wrote:

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


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9211 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABG_p1DBzy4osKvZU-UUgL0MgmkzRQZwks5qJzkzgaJpZM4Iwdmf
.

@miminar
Copy link

miminar commented Jun 9, 2016

I don't think this will work with go 1.5 without exporting GO15VENDOREXPERIMENT=1 as well.

@miminar
Copy link

miminar commented Jun 9, 2016

This has been addressed already by @legionus in #8938 (on review). See 4390dad for exported GO15VENDOREXPERIMENT=1. Nevertheless, I'm fine with merging this before #8938.

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Jun 9, 2016 via email

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Jun 9, 2016 via email

@mfojtik
Copy link
Contributor

mfojtik commented Jun 9, 2016

@smarterclayton can you and @legionus talk about merging the efforts so @legionus don't have to rework his PR from scratch?

@smarterclayton
Copy link
Contributor Author

His PR only has to handle the parts that this handles - specifically, the GCS changes. His PR doesn't have to change dramatically.

I need this in - is there any feedback on this approach at this point?

@smarterclayton
Copy link
Contributor Author

@mfojtik this is going to go in before the rebase.

@miminar
Copy link

miminar commented Jun 9, 2016

LGTM

@smarterclayton
Copy link
Contributor Author

[merge]

@mfojtik
Copy link
Contributor

mfojtik commented Jun 9, 2016

@smarterclayton I think this will break me terribly, but rebase made me stronger and I can survive... so I'm sending you the rage face: :rage3:

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 9, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4623/) (Image: devenv-rhel7_4342)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 005c45d

@smarterclayton
Copy link
Contributor Author

You should just be able to grab the carry in - unless you changed Docker
distribution???

On Thu, Jun 9, 2016 at 11:19 AM, Michal Fojtik [email protected]
wrote:

@smarterclayton https://github.com/smarterclayton I think this will
break me terribly, but rebase made me stronger and I can survive... so I'm
sending you the rage face: [image: :rage3:]


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9211 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p82uO_TGWIuIn_BC66HIhV13g6o-ks5qKC71gaJpZM4Iwdmf
.

@mfojtik
Copy link
Contributor

mfojtik commented Jun 9, 2016

@smarterclayton i don't think I did, but I think you're right.

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.

6 participants