Refactor artifacts logic #91

justinsb · 2021-02-16T15:22:56Z

In particular this makes the distinction between the artifacts dir and the
run dir (but doesn't change the logic yet), but also allows us to download
and run ginkgo without requiring it to be in the PATH (if ARTIFACTS is not
set).

Builds on #90

amwat

Huge +1 to this, Thanks!

Ideally we want to expose different options, so deployers can decide for themselves which artifacts are

ephemeral (e.g. os.TempDir() ),
not ephemeral but not upload-worthy (RunDir()),
not ephemeral, usable across runs (os.UserCacheDir() )
not ephemeral, upload-worthy (ArtifactsDir()).

/lgtm
/approve
/hold for a few nits

amwat · 2021-02-17T04:18:48Z

pkg/app/cmd.go

@@ -260,14 +233,14 @@ func (o *options) ShouldTest() bool {
 	return o.test != ""
 }

-func (o *options) ArtifactsDir() string {


we might need to expose this again in the future if this and rundir are decoupled.

Agree, but now we should use artifacts.BaseDir() (note that we've moved the flag there, invoked by artifacts.BindFlags())

amwat · 2021-02-17T04:20:25Z

pkg/app/app.go

@@ -120,7 +120,7 @@ func RealMain(opts types.Options, d types.Deployer, tester types.Tester) (result
 		exec.InheritOutput(test)

 		envsForTester := os.Environ()
-		envsForTester = append(envsForTester, fmt.Sprintf("%s=%s", "ARTIFACTS", opts.ArtifactsDir()))
+		envsForTester = append(envsForTester, fmt.Sprintf("%s=%s", "ARTIFACTS", opts.RunDir()))


let's also additionally expose this as KUBETEST2_RUN_DIR

I like that - setting us up for the future cleanup (I like the 4 directories you suggested in your comment - just wanted to keep the PR focused). Added!

amwat · 2021-02-17T04:26:49Z

pkg/testers/ginkgo/package.go

@@ -72,7 +73,12 @@ func (t *Tester) AcquireTestPackage() error {
 }

 func (t *Tester) extractBinaries(downloadPath string) error {
-	// finally, search for the test package and extract it
+	// ensure the artifacts dir
+	if err := os.MkdirAll(artifacts.BaseDir(), os.ModePerm); err != nil {


I think we already do this as part of the main kubetest2 app code.

Yes, but kubetest2-test-kops calls this directly

amwat · 2021-02-17T04:28:04Z

pkg/app/app.go

@@ -57,12 +57,12 @@ func RealMain(opts types.Options, d types.Deployer, tester types.Tester) (result
 	// TODO(bentheelder): signal handling & timeout

 	// ensure the artifacts dir
-	if err := os.MkdirAll(opts.ArtifactsDir(), os.ModePerm); err != nil {
+	if err := os.MkdirAll(opts.RunDir(), os.ModePerm); err != nil {


we ensure the directory exists, can drop from ginkgo tester.

nit: Also update that comment on 59 to match*

Do we ensure it exists with correct permissions?

@MushuEE Updated the comment.

@amwat I actually added it to the ginkgo tester because I hit the directory not existing, because the ginkgo tester might be invoked from e.g. kubetest2-tester-kops and doesn't necessarily go through this path (though I'm still learning how this all fits together, so could be wrong here).

hmm if you invoke the kubetest2-tester-kops binary directly (not recommended) then you would hit this.
but if you invoke it as kubetest2 kops --test=kops then it should go through the kubetest2 lifecycle.

/hold cancel

interested in getting this in, can be cleaned up later.

amwat · 2021-02-17T04:30:38Z

pkg/app/app.go

@@ -57,12 +57,12 @@ func RealMain(opts types.Options, d types.Deployer, tester types.Tester) (result
 	// TODO(bentheelder): signal handling & timeout

 	// ensure the artifacts dir
-	if err := os.MkdirAll(opts.ArtifactsDir(), os.ModePerm); err != nil {
+	if err := os.MkdirAll(opts.RunDir(), os.ModePerm); err != nil {


nit: since we are already changing stuff related to this
let's klog.Infof(opts.RunDir()) similar to how we log the runID on lin 87 for consistency

Good idea - done!

k8s-ci-robot · 2021-02-17T04:32:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amwat, justinsb

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~~ [amwat]

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

MushuEE · 2021-02-17T17:34:25Z

pkg/app/app.go

@@ -57,12 +57,12 @@ func RealMain(opts types.Options, d types.Deployer, tester types.Tester) (result
 	// TODO(bentheelder): signal handling & timeout

 	// ensure the artifacts dir
-	if err := os.MkdirAll(opts.ArtifactsDir(), os.ModePerm); err != nil {
+	if err := os.MkdirAll(opts.RunDir(), os.ModePerm); err != nil {


nit: Also update that comment on 59 to match*

Do we ensure it exists with correct permissions?

MushuEE · 2021-02-17T17:38:03Z

pkg/testers/ginkgo/package.go

 	for {
 		// Put this check before any break condition so we don't
 		// accidentally incorrectly error
-		if foundTestPackage && foundBinary {
+		if len(extracted) == len(extract) {


Do you want to return here or break to also get the double check on the downloadPath check?

Good call - I think I get away with it, but a break is cleaner. Changed!

justinsb

Thanks - pushed updates!

justinsb · 2021-02-19T15:10:14Z

pkg/app/app.go

@@ -57,12 +57,12 @@ func RealMain(opts types.Options, d types.Deployer, tester types.Tester) (result
 	// TODO(bentheelder): signal handling & timeout

 	// ensure the artifacts dir
-	if err := os.MkdirAll(opts.ArtifactsDir(), os.ModePerm); err != nil {
+	if err := os.MkdirAll(opts.RunDir(), os.ModePerm); err != nil {


@MushuEE Updated the comment.

@amwat I actually added it to the ginkgo tester because I hit the directory not existing, because the ginkgo tester might be invoked from e.g. kubetest2-tester-kops and doesn't necessarily go through this path (though I'm still learning how this all fits together, so could be wrong here).

justinsb · 2021-02-19T15:10:46Z

pkg/app/app.go

@@ -57,12 +57,12 @@ func RealMain(opts types.Options, d types.Deployer, tester types.Tester) (result
 	// TODO(bentheelder): signal handling & timeout

 	// ensure the artifacts dir
-	if err := os.MkdirAll(opts.ArtifactsDir(), os.ModePerm); err != nil {
+	if err := os.MkdirAll(opts.RunDir(), os.ModePerm); err != nil {


Good idea - done!

justinsb · 2021-02-19T15:11:31Z

pkg/app/app.go

@@ -120,7 +120,7 @@ func RealMain(opts types.Options, d types.Deployer, tester types.Tester) (result
 		exec.InheritOutput(test)

 		envsForTester := os.Environ()
-		envsForTester = append(envsForTester, fmt.Sprintf("%s=%s", "ARTIFACTS", opts.ArtifactsDir()))
+		envsForTester = append(envsForTester, fmt.Sprintf("%s=%s", "ARTIFACTS", opts.RunDir()))


I like that - setting us up for the future cleanup (I like the 4 directories you suggested in your comment - just wanted to keep the PR focused). Added!

justinsb · 2021-02-19T15:13:41Z

pkg/app/cmd.go

@@ -260,14 +233,14 @@ func (o *options) ShouldTest() bool {
 	return o.test != ""
 }

-func (o *options) ArtifactsDir() string {


Agree, but now we should use artifacts.BaseDir() (note that we've moved the flag there, invoked by artifacts.BindFlags())

justinsb · 2021-02-19T15:14:12Z

pkg/testers/ginkgo/package.go

@@ -72,7 +73,12 @@ func (t *Tester) AcquireTestPackage() error {
 }

 func (t *Tester) extractBinaries(downloadPath string) error {
-	// finally, search for the test package and extract it
+	// ensure the artifacts dir
+	if err := os.MkdirAll(artifacts.BaseDir(), os.ModePerm); err != nil {


Yes, but kubetest2-test-kops calls this directly

justinsb · 2021-02-19T15:14:37Z

pkg/testers/ginkgo/package.go

 	for {
 		// Put this check before any break condition so we don't
 		// accidentally incorrectly error
-		if foundTestPackage && foundBinary {
+		if len(extracted) == len(extract) {


Good call - I think I get away with it, but a break is cleaner. Changed!

In particular this makes the distinction between the artifacts dir and the run dir (but doesn't change the logic yet), but also allows us to download and run ginkgo without requiring it to be in the PATH (if ARTIFACTS is not set).

amwat · 2021-02-24T06:42:02Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 16, 2021

k8s-ci-robot requested review from cofyc and MushuEE February 16, 2021 15:23

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 16, 2021

amwat reviewed Feb 17, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 17, 2021

k8s-ci-robot assigned amwat Feb 17, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 17, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 17, 2021

MushuEE reviewed Feb 17, 2021

View reviewed changes

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

justinsb force-pushed the refactor_artifacts_logic branch from 2562ddf to 5440fc8 Compare February 19, 2021 15:15

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2021

justinsb commented Feb 19, 2021

View reviewed changes

Refactor artifacts logic

7f5e6db

In particular this makes the distinction between the artifacts dir and the run dir (but doesn't change the logic yet), but also allows us to download and run ginkgo without requiring it to be in the PATH (if ARTIFACTS is not set).

justinsb force-pushed the refactor_artifacts_logic branch from 5440fc8 to 7f5e6db Compare February 19, 2021 15:17

k8s-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 19, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2021

k8s-ci-robot merged commit d152169 into kubernetes-sigs:master Feb 24, 2021

amwat mentioned this pull request Mar 1, 2021

Decouple RunDir from Artifacts #98

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor artifacts logic #91

Refactor artifacts logic #91

justinsb commented Feb 16, 2021

amwat left a comment

amwat Feb 17, 2021

justinsb Feb 19, 2021

amwat Feb 17, 2021

justinsb Feb 19, 2021

amwat Feb 17, 2021

justinsb Feb 19, 2021

amwat Feb 17, 2021

MushuEE Feb 17, 2021

justinsb Feb 19, 2021

amwat Feb 19, 2021

amwat Feb 24, 2021

amwat Feb 17, 2021

justinsb Feb 19, 2021

k8s-ci-robot commented Feb 17, 2021

MushuEE Feb 17, 2021

MushuEE Feb 17, 2021

justinsb Feb 19, 2021

justinsb left a comment

justinsb Feb 19, 2021

justinsb Feb 19, 2021

justinsb Feb 19, 2021

justinsb Feb 19, 2021

justinsb Feb 19, 2021

justinsb Feb 19, 2021

amwat commented Feb 24, 2021

Refactor artifacts logic #91

Refactor artifacts logic #91

Conversation

justinsb commented Feb 16, 2021

amwat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amwat commented Feb 24, 2021