move pkg to cmds #6399

rajatjindal · 2019-01-25T11:36:41Z

an ambitious try to move commands to a different package then 'main' to be able to use them as library.

k8s-ci-robot · 2019-01-25T11:36:48Z

Hi @rajatjindal. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2019-01-25T11:37:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rajatjindal
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: chrislovecnm

If they are not already assigned, you can assign the PR to them by writing /assign @chrislovecnm in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

zetaab · 2019-01-25T12:07:01Z

/ok-to-test

rajatjindal · 2019-01-25T13:55:34Z

/test pull-kops-bazel-test

chrisz100 · 2019-01-27T21:13:58Z

I'm wondering what your goal with publishing the commands as a library is? That's quite a huge change...

rajatjindal · 2019-01-28T06:24:45Z

Hi @chrisz100

thanks for looking at the PR.

I want to be able to use the commands from the wrapper i've on top of kops. today we exec kops command from it and if we could make use of commands directly, it will simplify a few things for us.

this is just like how kubectl exposes its commands as library and kops make use of them in its implementation.

does that make sense? and do u think its reasonable (i am hoping we don't consider it breaking change as these commands were never exposed to user anyways).

Thanks
Rajat Jindal

rajatjindal · 2019-01-29T02:35:52Z

Hi @chrisz100

do you think this change can be merged. If not we can close the PR and I will look for other ways to do what I am trying to do.

If you think it can be merged, I will rebase it against master.

Many thanks for considering this PR.

Rajat Jindal

chrisz100 · 2019-01-31T19:50:49Z

@rajatjindal makes sense - by the way no need to be so formal. It's all just people eager to move this project forward here :-)

It's quite a huge PR, do you have some information on how you tested everything kops does still works?

chrisz100 · 2019-01-31T20:11:39Z

README.md

@@ -1,4 +1,4 @@
-<img src="/docs/img/logo.jpg" width="500px" alt="kops logo">
+<img src="/docs/img/logo.jpg" width="500px" alt="kops logo"> 


Don't really see this as a change that should be in this PR

i think it was to trigger ci again as it flaked on a test. i will remove it.

chrisz100 · 2019-01-31T20:12:08Z

cmd/kops/BUILD.bazel

@@ -1,131 +1,11 @@
-load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library", "go_test")
+load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")


I'm not an expert in bazel. @justinsb is this good to remove?

i think this is generated. i didn't explicitly removed it.

drekle · 2019-01-31T20:38:03Z

This kind of steps away from a traditional folder structure that you see in most go projects. When I am looking at a project I expect most libraries to be under /pkg. While there is no universally accepted standard for this, it seems common in most of the repos.

Consider:
https://github.com/golang-standards/project-layout

Also maybe refer to how its done here, which I find a little cleaner: https://github.com/kubernetes/kubernetes/blob/master/cmd/kubectl/kubectl.go#L29

I don't think the PR is that ambitious really. Just in the wrong place.

drekle · 2019-01-31T20:39:54Z

cmd/kops/cmds/create_cluster.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package main
+package cmds


Consider mimicking the following layout instead.

https://github.com/kubernetes/kubernetes/blob/master/cmd/kubectl/kubectl.go#L29

Generally agree but the kops project is so spread I doubt starting with this would help to clean this up. @mikesplain @justinsb what do you say?

requesting feedback from @mikesplain @justinsb

As this line refers to the package name and does not import anything this looks perfectly fine to me.

All projects do it as proposed here, see:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubectl/kubectl.go#L17

zetaab · 2019-02-07T21:38:51Z

from my perspective this PR is really interesting and I would like to see progress in this. How we could get this forward? I know its big change, but this allows use kops as library. Now the situation is that if we want use kops as library we need copy paste quite much stuff.

zetaab · 2019-02-27T06:52:43Z

@justinsb @mikesplain could you please give your opinion here? At least for me this PR looks very interesting because I would like to use kops as library from another code.

chrisz100 · 2019-03-06T08:55:35Z

Can you please rebase and fix the tests @rajatjindal ? Also, is this WIP still? When do you plan to have this ready?

rajatjindal · 2019-03-06T10:04:57Z

I wanted to make sure this PR is acceptable to other core members of the project. I will rebase shortly and look into the test failure as well. thanks.

zetaab · 2019-03-06T19:43:31Z

@rajatjindal at least I want this PR. I am planning use kops as part of webservice

rajatjindal · 2019-03-12T08:21:08Z

Hi @zetaab @chrisz100

I've rebased the PR and fixed the tests. can you please sponsor this PR with other core members of the project and help it move forward.

Many thanks
Rajat Jindal

justinsb · 2019-03-15T04:15:09Z

This seems fine in general, though it's going to be hard to get it to a point where there are no conflicts, and we do have to prioritize bug fixes and the like.

I would like to see this though - I'm really pushing to get 1.12.0-alpha.1 done, and then hopefully things will settle down a little bit and then it won't be a constant rebase battle.

k8s-ci-robot · 2019-03-16T23:27:34Z

@rajatjindal: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fejta-bot · 2019-06-14T23:50:01Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

rajatjindal · 2019-06-15T10:38:09Z

Hi @justinsb do you think this PR stand a chance to get merged now if I rebase it against master now?

fejta-bot · 2019-07-15T10:48:46Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2019-08-14T11:32:33Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2019-08-14T11:32:41Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 25, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 25, 2019

k8s-ci-robot requested review from rdrgmnzs and robinpercy January 25, 2019 11:37

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2019

chrisz100 reviewed Jan 31, 2019

View reviewed changes

drekle suggested changes Jan 31, 2019

View reviewed changes

drekle mentioned this pull request Feb 5, 2019

REQUEST: New membership for drekle kubernetes/org#448

Closed

6 tasks

rajatjindal force-pushed the fix-pkg-name branch from 98ce6a0 to 4227c3f Compare March 6, 2019 10:30

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2019

rajatjindal force-pushed the fix-pkg-name branch 3 times, most recently from 64df6bf to aac02a7 Compare March 12, 2019 04:09

move pkg to cmds

2e297ab

rajatjindal force-pushed the fix-pkg-name branch from 8b7bf75 to 2e297ab Compare March 12, 2019 04:50

rajatjindal changed the title ~~[WIP] move pkg to cmds~~ move pkg to cmds Mar 12, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 12, 2019

justinsb added this to the 1.12 milestone Mar 15, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2019

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 14, 2019

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 15, 2019

k8s-ci-robot closed this Aug 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move pkg to cmds #6399

move pkg to cmds #6399

rajatjindal commented Jan 25, 2019

k8s-ci-robot commented Jan 25, 2019

k8s-ci-robot commented Jan 25, 2019

zetaab commented Jan 25, 2019

rajatjindal commented Jan 25, 2019

chrisz100 commented Jan 27, 2019

rajatjindal commented Jan 28, 2019

rajatjindal commented Jan 29, 2019

chrisz100 commented Jan 31, 2019

chrisz100 Jan 31, 2019

rajatjindal Feb 8, 2019 •

edited

Loading

chrisz100 Jan 31, 2019

rajatjindal Feb 8, 2019

drekle commented Jan 31, 2019 •

edited

Loading

drekle Jan 31, 2019 •

edited

Loading

chrisz100 Jan 31, 2019

rajatjindal Feb 8, 2019

chrisz100 Mar 6, 2019

zetaab commented Feb 7, 2019

zetaab commented Feb 27, 2019

chrisz100 commented Mar 6, 2019

rajatjindal commented Mar 6, 2019

zetaab commented Mar 6, 2019

rajatjindal commented Mar 12, 2019

justinsb commented Mar 15, 2019

k8s-ci-robot commented Mar 16, 2019

fejta-bot commented Jun 14, 2019

rajatjindal commented Jun 15, 2019

fejta-bot commented Jul 15, 2019

fejta-bot commented Aug 14, 2019

k8s-ci-robot commented Aug 14, 2019

		@@ -1,4 +1,4 @@
		<img src="/docs/img/logo.jpg" width="500px" alt="kops logo">
		<img src="/docs/img/logo.jpg" width="500px" alt="kops logo">

		@@ -1,131 +1,11 @@
		load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library", "go_test")
		load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")

move pkg to cmds #6399

move pkg to cmds #6399

Conversation

rajatjindal commented Jan 25, 2019

k8s-ci-robot commented Jan 25, 2019

k8s-ci-robot commented Jan 25, 2019

zetaab commented Jan 25, 2019

rajatjindal commented Jan 25, 2019

chrisz100 commented Jan 27, 2019

rajatjindal commented Jan 28, 2019

rajatjindal commented Jan 29, 2019

chrisz100 commented Jan 31, 2019

chrisz100 Jan 31, 2019

Choose a reason for hiding this comment

rajatjindal Feb 8, 2019 • edited Loading

Choose a reason for hiding this comment

chrisz100 Jan 31, 2019

Choose a reason for hiding this comment

rajatjindal Feb 8, 2019

Choose a reason for hiding this comment

drekle commented Jan 31, 2019 • edited Loading

drekle Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

chrisz100 Jan 31, 2019

Choose a reason for hiding this comment

rajatjindal Feb 8, 2019

Choose a reason for hiding this comment

chrisz100 Mar 6, 2019

Choose a reason for hiding this comment

zetaab commented Feb 7, 2019

zetaab commented Feb 27, 2019

chrisz100 commented Mar 6, 2019

rajatjindal commented Mar 6, 2019

zetaab commented Mar 6, 2019

rajatjindal commented Mar 12, 2019

justinsb commented Mar 15, 2019

k8s-ci-robot commented Mar 16, 2019

fejta-bot commented Jun 14, 2019

rajatjindal commented Jun 15, 2019

fejta-bot commented Jul 15, 2019

fejta-bot commented Aug 14, 2019

k8s-ci-robot commented Aug 14, 2019

rajatjindal Feb 8, 2019 •

edited

Loading

drekle commented Jan 31, 2019 •

edited

Loading

drekle Jan 31, 2019 •

edited

Loading