Start sandbox logic shared for demo/sandbox cmds #minor

[dav009]

TL;DR

Logic for start sandbox and demo is redudant. This PR unifies start logic under a pkg/sandbox package

Type

• Bug Fix
• Feature
• Plugin

Are all requirements met?

• Code completed
• Smoke tested
• Unit tests added
• Code documentation added
• Any pending items have an associated Issue

Complete description

As tracking issue describes there is a lot of repeated logic between sandbox and demo commands. This PR unifies the logic for starting a sandbox under a new package pkg/sandbox. Upcoming Prs will address teardown and other subcommands.

This PR:

• Unifies logic for starting demo and sandbox
• Introduces a new package pkg/sandbox
• Starting a demo is starting a sandbox with: (i) a particular docker image, (ii) prime pods running.
• Config for sandbox now lives in pkg/sandbox
• ImagePullPolicy and ImagePullOptions are part of pkg/docker
• Most of this PR is just moving code .
• Tests are mostly kept as they were
• added StartDemoCluster and StartSandboxCluster which rely on calling StartCluster with needed params for each particular case

⚠️ I still have to create/destroy and check both demo and sandboxes runs locally. Tests and lints are passing at the moment.

Whats next?

In other batch of work/PR:

• do the same for teardown
• do the same for status

Tracking Issue

flyteorg/flyte#2472

[codecov]

Codecov Report

Merging #326 (21df2ae) into master (bff8cf1) will decrease coverage by 0.55%.
The diff coverage is 74.61%.

@@            Coverage Diff             @@
##           master     #326      +/-   ##
==========================================
- Coverage   70.03%   69.48%   -0.56%     
==========================================
  Files         138      139       +1     
  Lines        4933     4824     -109     
==========================================
- Hits         3455     3352     -103     
- Misses       1221     1230       +9     
+ Partials      257      242      -15     

Flag	Coverage Δ
unittests	69.04% <73.18%> (-0.44%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/demo/start.go	0.00% <0.00%> (-78.49%)	⬇️
cmd/sandbox/start.go	0.00% <0.00%> (-78.84%)	⬇️
pkg/docker/docker.go	0.00% <0.00%> (ø)
pkg/docker/imagepullpolicy_enumer.go	0.00% <ø> (ø)
pkg/sandbox/start.go	78.88% <78.88%> (ø)
cmd/demo/demo.go	100.00% <100.00%> (ø)
cmd/sandbox/sandbox.go	100.00% <100.00%> (ø)
pkg/docker/docker_util.go	77.96% <100.00%> (ø)
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bff8cf1...21df2ae. Read the comment docs.

[pmahindrakar-oss]

LGTM . Please update on the test results and we can get this checkedin.

[pmahindrakar-oss]

The pattern used in flytectl is to define all configs for the subcommands in the config;/subcommand/
Can we keep the same structure.

[dav009]

@pmahindrakar-oss my motivation for moving this struct here is that I made this part of the inputs to some of the functions in pgk/sandbox. As shown in the signatures below

func StartCluster(ctx context.Context, args []string, sandboxConfig *Config, primePod bool, defaultImageName string)
func StartDemoCluster(ctx context.Context, args []string, sandboxConfig *Config)

what do you think it is better?

• just pass all of this fields directly into these functions
• define this config in config/subcommand and then import it in pkg/sanbox? ( I wonder if this might cause an import loop)
• create another struct essentially with the same fields. Maybe we can embed these structs together?

// embeding Config in CLIConfig
// lives in cmd/config/subcommand
type CLIConfig struct {
   // sandbox.Config
   Config
}

[pmahindrakar-oss]

Option 2 & 3 should be good.
For the option 2 . I dont see an import loop issue since

subcommand is only imported in cmd module
pkg is imported in cmd module
here you are introducing a dependency from subcommand -> pkg which should be fine.

Option 3 might lead to issue with pflag generation though as embedded struct may not be supported but you can check.

[dav009]

@pmahindrakar-oss

• moved config to cmd/config
• ran flytectl demo start and flytectl sandbox start locally succesfully. 🟢
• Fixed a bunch of issues that I missed on the way:
  • essentially passing different ports (yeah, ports are different for demo and sandbox)
  • different way to look for a fully qualified docker image

---

codecov looks very sad now.
Maybe we could have another good first issue to make StartSandboxCluster and StartDemoCluster better testable, it is hard to mock things and verify function calls.

I was thinking on a struct/interface like:

type Sandbox struct{
  start : func(...)
  teardown: func(...)
}

and then we would be able to mock and tests what values other functions pass

[pmahindrakar-oss]

Yes we can do the tests as followup.Thanks for making this change