search

kubernetes-sigs / kubetest2

Kubetest2 is the framework for launching and running end-to-end tests on Kubernetes.
Apache License 2.0
331 stars 106 forks source link

Decouple RunDir from Artifacts #183

Closed Rajalakshmi-Girish closed 2 years ago

Rajalakshmi-Girish commented 2 years ago

Fixes https://github.com/kubernetes-sigs/kubetest2/issues/98 This change separates RunDir from Artifacts and introduces a flag cp-rundir-to-artifacts for intentionally copying binaries/ metadata to artifacts.

Rajalakshmi-Girish commented 2 years ago

@mkumatag ^^

mkumatag commented 2 years ago

/cc @amwat

mkumatag commented 2 years ago

/assign @spiffxp

Rajalakshmi-Girish commented 2 years ago

@spiffxp Can you please take a look when you have some time?

BenTheElder commented 2 years ago

spiffxp is not available ATM and won't be for some time.

Rajalakshmi-Girish commented 2 years ago

spiffxp is not available ATM and won't be for some time.

Thanks for letting us know. Do you have some time to review this, please? :)

Rajalakshmi-Girish commented 2 years ago

@BenTheElder I have incorporated your comments from the previous review.

These changes were verified for the following scenarios:

When environment variable KUBETEST2_RUN_DIR is set 

1. `cp-rundir-to-artifacts` flag is set to `True`
2. `cp-rundir-to-artifacts` flag is not set.

KUBETEST2_RUN_DIR is not set 

3. value passed in flag `--rundir` with `cp-rundir-to-artifacts` flag is set to `True`
4. value passed in flag `--rundir` with `cp-rundir-to-artifacts` flag is not set.

When KUBETEST2_RUN_DIR is not set and --rundir is not passed.

5. cp-rundir-to-artifacts` flag is set to `True`
6. `cp-rundir-to-artifacts` flag is not set.

@mkumatag ^^

MushuEE commented 2 years ago

/lgtm

BenTheElder commented 2 years ago

thanks for working through figuring out the best approach on this and keeping after the PR, even with all the delayed reviews etc 🙏

still looking to get more folks involved in reviewing and approving here, I may have to refocus soon 😅

Rajalakshmi-Girish commented 2 years ago

something like git rebase -i HEAD~2, replace pick => f (for fixup) for the second commit

Thank you. I shall do it.

Rajalakshmi-Girish commented 2 years ago

do you mind rebasing to fixup/squash / avoid the merge commit? just to keep the history clean.

I am finding it difficult to rebase and squash those commits :( Not sure why, but git rebase -i HEAD~2 is not displaying the commit that I want to squash. Will check this tomorrow.

Rajalakshmi-Girish commented 2 years ago

@BenTheElder Can you please take a look?

k8s-ci-robot commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, Rajalakshmi-Girish

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](https://github.com/kubernetes-sigs/kubetest2/blob/master/OWNERS)~~ [BenTheElder] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment