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

Propose a test frameworks git repo #1524

Closed
wants to merge 4 commits into from
Closed

Conversation

totherme
Copy link

@totherme totherme commented Dec 18, 2017

Following up on this email thread, this is a proposal for a new git repository to house:

  • the integration testing framework which is currently being built in the kubectl repo
  • possibly also the framework currently in kubernetes/test/e2e/framework

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 18, 2017
@k8s-github-robot k8s-github-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Dec 18, 2017
@stevekuznetsov
Copy link
Contributor

/cc @marun

We propose a new git repository named `k8s.io/testframeworks`. This would be
backed by a github repository `github.com/kubernetes/testframeworks`.

### What should live in it?
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to make explicit which other k8s.io/ repos this would be allowed to import

Copy link
Author

Choose a reason for hiding this comment

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

Good call. Which repos would folks like? :)

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, none.

Copy link
Contributor

Choose a reason for hiding this comment

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

But the e2e might depend on the client? (Or does it shell out?)

Copy link
Member

Choose a reason for hiding this comment

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

If it does, then I don't think it can live outside of k/k.

avoid characters that are not lower case alphabetic characters. This precludes
for example `k8s.io/test-frameworks`.

### Who should own it?
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, what does ownership mean?

Copy link
Author

Choose a reason for hiding this comment

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

In this email when @spiffxp said "would naturally fall under the ownership of sig-testing", I presumed that "ownership" effectively meant something like "whose names go in the top level OWNERS file". I may have been wrong...

Copy link
Member

Choose a reason for hiding this comment

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

I would expect the individuals proposing to work on this project to be the leads for it, and responsible for actually driving / implementing it. This is not a thing I have the capacity to staff or drive. So I'd like to see the "implementation owner TBD" part filled out.

But in the context of SIG's needing to own all things, in the same way that SIG Apps "owns" helm and charts, I think it makes sense for SIG Testing to own this. I would like to hear from @timothysc if this aligns with his proposal to expand the charter of sig-testing but I'm personally +1 here

Copy link
Author

Choose a reason for hiding this comment

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

I think the "individuals proposing to work on this project" are initially me, @hoegaarden and @apelisse. So far all the PRs on the integration framework have been authored by me and @hoegaarden, and reviewed by @apelisse.

I can't speak with as much certainty for @apelisse , but I know that @hoegaarden and I are happy to be responsible for driving/implementing this framework so long as that makes sense for everyone else.

Copy link
Author

Choose a reason for hiding this comment

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

I've added our (my, @apelisse, and @hoegaarden's) names to the proposal, and clarified my understanding of the difference between the "SIG's needing to own all things" context, and the "day to day looking after the code" context. Does that work for everyone?

@spiffxp
Copy link
Member

spiffxp commented Dec 18, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 18, 2017
Specifically in the light of [this
conversation](kubernetes#1524 (comment))
@spiffxp
Copy link
Member

spiffxp commented Dec 21, 2017

/approve
/hold
I am happy with this, but would like to see an /lgtm from @timothysc (or some other sig-testing lead)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 21, 2017
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: spiffxp, totherme

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 21, 2017
@stevekuznetsov
Copy link
Contributor

I think we may want to frame specifically which other repos this framework could import, if any, and enumerate them in the proposal. As it's meant to be helping with import cycles, that seems important to me as part of the charter for the repo.

@apelisse
Copy link
Member

apelisse commented Dec 21, 2017

I can't edit the pull-request and its authors are on vacation. I'd still really like the repo to exists when they come back.

I'll mention it here again, and I think it definitely should be printed in bold in the README at the root of the repo, but that new repo will NOT import any other kubernetes repository (maybe k8s.io/utils would be the exception).

@totherme
Copy link
Author

totherme commented Jan 2, 2018

I've added a section saying explicitly that no k8s repos other than utils should be imported. I know for sure that the CLI integration testing framework which originally motivated this PR does not import any kubernetes repos.

I can see (using ag k8s.io) that there seem to be plenty of files in kubernetes/test/e2e/framework that import apimachinery, client-go, api/extensions, pkg/kubelet, pkg/cloudprovider, apiserver, and so on. I therefore suspect that when @spiffxp said "Getting kubernetes/test/e2e/framework out into a reusable place is something I keep hearing a desire for", they probably meant that it might be a good idea to have space available in case e2e is able to make the move at some point in future. I think they didn't mean that we should whitelist all those packages for import. Have I understood you ok @spiffxp ?

@spiffxp
Copy link
Member

spiffxp commented Jan 2, 2018

@totherme you have understood correctly, thanks


## Proposal

We propose a new git repository named `k8s.io/testframeworks`. This would be
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer just to call it the testing repo. b/c there could also be separate libraries and possible binary artifacts.

There also [seems to be a
desire](https://groups.google.com/a/kubernetes.io/d/msg/steering/LA9WiFnl6PI/os48-c3HCgAJ)
to get `kubernetes/test/e2e/framework` out into a more reusable place. It could
live in `k8s.io/testframeworks/e2e`
Copy link
Member

Choose a reason for hiding this comment

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

There are a number of test-specific utility libraries that should be housed in this repo.


### What repos can be imported (and hence vendored) into this repo?

No kubernetes repos other than [utils](https://github.com/kubernetes/utils)
Copy link
Member

Choose a reason for hiding this comment

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

you'll absolutely need either the internal or external client.

part of kubernetes has to do so through some CLI. The [CLI integration testing
framework](https://github.com/kubernetes/kubectl/tree/master/pkg/framework/test)
which originally motivated this repo certainly does not have any kubernetes
dependencies.
Copy link
Member

Choose a reason for hiding this comment

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

That's not possible, but it is possible to ensure it's a proper DAG.

@timothysc
Copy link
Member

This falls in line with the subgroup proposed for sig-testing that solely focuses on tests, and code organization as well.

@spiffxp
Copy link
Member

spiffxp commented Jan 5, 2018

@timothysc @stevekuznetsov I'd like to leave it to you two to decide how much of this needs to be hashed out before /lgtm'ing

In the meantime, I'm working on creating https://github.com/kubernetes-sig-testing to host a repo called "frameworks" that would live as testing.k8s.io/frameworks per https://groups.google.com/forum/#!topic/kubernetes-sig-testing/8vacWVjIXCg

@timothysc
Copy link
Member

Per discussions, I'm ok with this, but I don't think we should merge this proposal and we should use this example to serve as a test case for @brendandburns proposal.

@spiffxp
Copy link
Member

spiffxp commented Jan 9, 2018

@timothysc I guess I would prefer to see this proposal amended to reference the repo I'm creating per said proposal, so there is written documentation somewhere explaining its motivation.

@totherme
Copy link
Author

I've amended this proposal to reference the repo that @spiffxp is creating, and to clarify my understanding of what happened in the recent sig-architecture meeting.

@timothysc
Copy link
Member

Sure, but lets not merge this unless we have to, in preference for vetting on the official doc.

@spiffxp
Copy link
Member

spiffxp commented Jan 17, 2018

in preference for vetting on the official doc

@timothysc I'm not clear on what this means, can you elaborate?

@marun
Copy link
Contributor

marun commented Feb 9, 2018

@timothysc: Github emailed me comments on this issue (dated Jan 9) from @sttts regarding how the new repo wouldn't be able to rely on any staging repos if it was to be consumed by k/k, but I don't see those comments reflected here. I think it may be worth reconsidering the use of an independent repo. I would like to ensure the evolution of test tooling for both k/k (where the bulk of testing work is currently done) and external repos (where the bulk of testing is likely to be done in the future), and from @sttts's comments only a repo staged out of k/k will accomplish both goals.

@timothysc
Copy link
Member

So it already exists and the spec is in flight for a general-ish solution, closing.

@timothysc timothysc closed this Apr 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/design Categorizes issue or PR as related to design. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants