Skip to content

Conversation

@deads2k
Copy link
Contributor

@deads2k deads2k commented Aug 11, 2017

This is an attempt using glide. I've managed to generate building code, so that's nice.

I used glide to handle the vendor dir, but I'm unsure how repeatable it really is.

The clients are under
<group>/{internalclientset,clientset,informers,listers}

@liggitt @sttts @smarterclayton let's argue about:

  1. whether we want to support go get-ability. I don't, vendor or you aren't repeatable and if you aren't repeatable, then I don't feel too bad about weird things happening
  2. whether we want to publish internal clients. I think internal clients could be useful for an orderly transition of our origin projects that could use this, but I wouldn't want to support them as stable.
  3. location of the generated code.

@smarterclayton
Copy link
Contributor

I think we want to support go get to a degree

@smarterclayton
Copy link
Contributor

In the sense that there's a reason people want to do go get.

I'm less worried about internal clients - I think evidence from upstream is that only a small subset of people needed them.

@sttts
Copy link
Contributor

sttts commented Aug 14, 2017

About 1: I don't care much about go-get'ability. But I care about integrating this into other projects. The procedure for kube/client-go (compare https://github.com/kubernetes/client-go/issues/259#issuecomment-320320158) is

  • do checkout client-go,
  • then godep restore from there,
  • go to your project and godep save.

Does this work with glide? Masterminds/glide#366 suggests it doesn't.

- external/github.com/docker/docker/pkg/archive
- external/github.com/docker/docker/pkg/fileutils
- external/github.com/docker/docker/pkg/homedir
- external/github.com/docker/docker/pkg/idtools
Copy link
Contributor

Choose a reason for hiding this comment

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

omg, we vendored the whole universe.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should find why we need those and make issues to improve our code hygiene for those in corresponding teams :)

@@ -0,0 +1,36 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not a fan of these 50 liner bash scripts in every projects right now. But it's the best we have right now :-/

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call it update-codegen.sh at least?

@@ -0,0 +1,36 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

#!/bin/bash -e for proper error handling.

openshift-merge-robot added a commit to openshift/origin that referenced this pull request Aug 28, 2017
Automatic merge from submit-queue (batch tested with PRs 15719, 15761)

moves the TSB into its own package

This moves all TSB code into its own package.  The only references we have to this code are here:

```
./pkg/cmd/server/start/start_allinone.go
./pkg/cmd/server/origin/master.go
./test/extended/templates/templateservicebroker_security.go
./test/extended/templates/templateservicebroker_e2e.go
```

We're going to remove the `master.go` reference in #15684, leaving a dependency on building the command and in the e2e tests.

@smarterclayton @sttts Are we ready to jump off the cliff and move to staging repos?  We'd end up with cyclical imports until we sort out openshift/client-go#2, but that looks tractable for an internal solution in 3.7 so we can learn how we want to shape it....
@deads2k
Copy link
Contributor Author

deads2k commented Sep 7, 2017

How about this as a starting point and after it merges I enable travis and we go from there?

@sttts
Copy link
Contributor

sttts commented Sep 7, 2017

As long as we leave the "Not ready yet" in the readme, lgtm.

@deads2k
Copy link
Contributor Author

deads2k commented Sep 7, 2017

As long as we leave the "Not ready yet" in the readme, lgtm.

Definitely will. Merging this to build on .

@deads2k deads2k merged commit dde2477 into openshift:master Sep 7, 2017
@mfojtik
Copy link
Contributor

mfojtik commented Sep 7, 2017

👍

1 similar comment
@ralphg6
Copy link

ralphg6 commented Sep 8, 2017

👍

eparis pushed a commit to eparis/client-go that referenced this pull request Apr 17, 2019
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.

5 participants