GIT-60: enable helper function for conditional waits

[harshanarayana]

Closes #60

[k8s-ci-robot]

Welcome @harshanarayana!

It looks like this is your first PR to kubernetes-sigs/e2e-framework 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/e2e-framework has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @harshanarayana. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

[vladimirvivien]

@harshanarayana This is a great start and captures most of everything I was thinking about. I left some comments because I think this can be refined further.

Design consideration

The main thing that I would like to see is to be able to invoke the For function from the wait package without the need to create a wait.Interface instance. One way that could be done is to create a new subpackage wait/cond where all pre-conditions are defined. The cond package would expose Condition value with all the pre-conditions defined

// package wait/cond
type Condition struct {
    res *resources.Resources
}
func New(*resources.Resrouces) *Condition
func (c *Condition) ResourceScaled(...)
func (c *Condition) JobMatche(...)
func (c *Condition)
...

Then, using the wait package could look like this:

import ".../.../wait"
import ".../.../wait/cond"

func DoK8sStuff(){
    pod := createPod(...)
    wait.For(cond.New(*resources.Resources).PodRunning(&pod))
}

Hope this makes sense.

[harshanarayana]

Hope this makes sense.

Absolutely. I like the notion of adding an inlined resource so that we can use multiple difference instance of the resources type to our advantage as required. Let me do the changes accordingly.

[harshanarayana]

@vladimirvivien I have addressed all the changes you suggested and refactored it accordingly. PTAL when you manage to get some time. Thanks

[vladimirvivien]

@harshanarayana First apologies for the late review.
Anyway, this is awesome work and you delivered exactly what the design called for.
I left a few comments (mostly housekeeping and cosmetics). Once done, this should be merged soon!!!!

[vladimirvivien]

These log statements will be a bit verbose when used. I would consider removing them.

[harshanarayana]

Can I make these optional? By default it will be turned off but can be enabled by the users if required. Would that be ok ?

[vladimirvivien]

I think outputting logs from the framework code could be an issue because, as a user, when I write tests, I don't expect extra output unless I use something like t.Log (etc).

As far as making it optional, here is what we can do:

• Get this merged (without logs)
• Create a new PR to introduce more robust log capabilities via a level log framework
• Then we can research for something that make sense for the project.
  How does that sound ?

[harshanarayana]

First apologies for the late review.

No worries at all @vladimirvivien . Thanks again for taking your time to review this. I have addressed the changes you requested. Let me know if you think anything else needs to be modified

[cpanato]

/ok-to-test

[vladimirvivien]

@harshanarayana
I still have reservation about how logging is done (it feels a bit one-off). I left comments.
Other than than that, we should be ready to merge soon.

[vladimirvivien]

@harshanarayana Awesome work.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshanarayana, vladimirvivien

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 [vladimirvivien]

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

[vladimirvivien]

@harshanarayana
Couple things I just realized we need

• Add an example test in the examples dir
• A quick documentation blurb in README.md
  Please open a new issue you or someone else can get that done.

Thanks again!!!

[harshanarayana]

@vladimirvivien let me take care of it right away.

[harshanarayana]

• Please open a new issue you or someone else can get that done.

#78

[helayoty]

This is a very needed feature, thanks @harshanarayana for the great effort. @vladimirvivien When do we expect a new release so we can use it?