extract RunCacheProbe function #284

johnstcn · 2024-07-26T16:32:14Z

Builds on top of #282 and #283

Extracts the logic for --get-cached-image to a separate function

Also pulls out some other common logic shared between Run and RunCacheProbe. There is more that could be DRYed up, but leaving this for later.

johnstcn · 2024-07-30T16:58:58Z

envbuilder.go

@@ -1137,3 +1407,111 @@ func copyFile(src, dst string) error {
 	}
 	return nil
 }
+


review: below functions are extracted

curious if we can move them to another unit/file

Yeah we probably should; I'd like to simplify the envbuilder.Run / RunCacheProbe functions as much as possible for future maintenance.

dannykopping

LGTM, mostly small nits and suggestions
Quite a lot of complexity in the RunCacheProbe function which would be good to decompose later on.

dannykopping · 2024-07-31T06:34:58Z

envbuilder.go

+// Logger is the logf to use for all operations.
+// Filesystem is the filesystem to use for all operations.
+// Defaults to the host filesystem.


Were these arguments previous, but you later moved them to options.Options by any chance?
This comment feels potentially stale.

Yep, this is stale 👍

envbuilder.go

dannykopping · 2024-07-31T06:37:49Z

envbuilder.go

+	stageNumber := 0
+	startStage := func(format string, args ...any) func(format string, args ...any) {
+		now := time.Now()
+		stageNumber++


Nit: pass in an int pointer instead perhaps?

This is a pre-existing non-pointer integer; I want to refactor this whole stage builder business later on. A little copying for now.

envbuilder.go

dannykopping · 2024-07-31T06:41:17Z

envbuilder.go

+			data := make([]byte, 4096)
+			for {
+				read, err := reader.Read(data)


Nit: consider using https://pkg.go.dev/bufio#NewReaderSize with ReadBytes(delim byte) where delim is '\r'?

pre-existing; will address in follow-up

envbuilder.go

mtojek

Please ignore comments that refer to the original code.

mtojek · 2024-07-31T09:45:12Z

envbuilder.go

+		return nil, fmt.Errorf("--cache-repo must be set when using --get-cached-image")
+	}
+
+	stageNumber := 0


nit: stageIndex?

will rename in a follow-up

mtojek · 2024-07-31T09:45:59Z

envbuilder.go

+			data := make([]byte, 4096)
+			for {
+				read, err := reader.Read(data)


envbuilder.go

mtojek · 2024-07-31T09:48:39Z

envbuilder.go

+			Insecure:      opts.Insecure,
+			InsecurePull:  opts.Insecure,
+			SkipTLSVerify: opts.Insecure,


Is it on purpose or is it something we should address in the future?

What is the issue here?

ignoring TLS issues

I think it's intentional here.

synced offline: I assumed incorrectly that it is SkipTLSVerify = true

mtojek · 2024-07-31T09:49:10Z

envbuilder.go

+	endStage("🏗️ Found cached image!")
+
+	// Sanitize the environment of any opts!
+	options.UnsetEnv()


nit: should run UnsetEnv in defer?

added at beginning of func

mtojek · 2024-07-31T09:49:38Z

envbuilder.go

@@ -1137,3 +1407,111 @@ func copyFile(src, dst string) error {
 	}
 	return nil
 }
+


curious if we can move them to another unit/file

mtojek · 2024-07-31T09:50:41Z

envbuilder.go

+
+func initDockerConfigJSON(dockerConfigBase64 string) (func() error, error) {
+	var cleanupOnce sync.Once
+	noop := func() error { return nil }


wouldn't it be more natural to just return nil and check it before execution?

Returning a noop function removes the need for the caller to care whether it's nil or not. I much prefer this pattern personally.

mtojek · 2024-07-31T09:52:58Z

envbuilder.go

+		return noop, fmt.Errorf("decode docker config: %w", err)
+	}
+	var configFile DockerConfig
+	decoded, err = hujson.Standardize(decoded)


;) tailscale/hujson#2

mtojek · 2024-07-31T09:54:30Z

envbuilder.go

+	// Spawn an in-memory registry to cache built layers...
+	registry := handlers.NewApp(ctx, cfg)
+
+	listener, err := net.Listen("tcp", "127.0.0.1:0")


nit: no need to expose it outside? I mean, 0.0.0.0:0 for debugging purposes?

I don't recall ever needing to do this; you can always inspect the local dir contents

mtojek · 2024-07-31T09:55:11Z

envbuilder.go

+	if opts.CacheRepo != "" {
+		destinations = append(destinations, opts.CacheRepo)
+	}
+	kOpts := &config.KanikoOptions{


At some point it would be great to treat Kaniko as strategy pattern, so we can replace it in the future.

Builds on top of #282 and #283: - Extracts the logic for --get-cached-image to a separate function - Also pulls out some other common logic shared between Run and RunCacheProbe. (cherry picked from commit cacbcb8)

johnstcn self-assigned this Jul 26, 2024

johnstcn force-pushed the cj/run-cache-probe branch from 4d002c7 to a75b84c Compare July 29, 2024 14:35

johnstcn changed the base branch from cj/option-defaults to cj/export-log July 29, 2024 14:53

johnstcn force-pushed the cj/export-log branch from 775d819 to 84f1f7b Compare July 29, 2024 16:37

Base automatically changed from cj/export-log to main July 29, 2024 16:56

extract RunCacheProbe function

3c8066f

johnstcn force-pushed the cj/run-cache-probe branch from a75b84c to 3c8066f Compare July 29, 2024 16:58

johnstcn added 2 commits July 30, 2024 16:58

extract CABundle and docker config JSON init functions

eeb25c1

extract local cache registry func

8349602

johnstcn marked this pull request as ready for review July 30, 2024 16:58

johnstcn requested review from mafredri, dannykopping and mtojek July 30, 2024 16:58

johnstcn commented Jul 30, 2024

View reviewed changes

dannykopping approved these changes Jul 31, 2024

View reviewed changes

johnstcn added 4 commits July 31, 2024 09:56

rm outdated comments

28aaec4

xerrors -> fmt

2d4b8fd

rm DoCacheProbe invocation in Run

8d39505

rm empty file

a7b4d39

mtojek reviewed Jul 31, 2024

View reviewed changes

johnstcn added 3 commits July 31, 2024 12:22

extract log.Writer func that allows stopping

ddfa66f

log errors cleaning up docker config json in defer

499bf17

defer options.UnsetEnv()

e15ecbc

mtojek approved these changes Jul 31, 2024

View reviewed changes

johnstcn merged commit cacbcb8 into main Jul 31, 2024
4 checks passed

johnstcn deleted the cj/run-cache-probe branch July 31, 2024 11:59

johnstcn mentioned this pull request Aug 1, 2024

envbuilder: address follow-on comments from GetCacheProbe refactor coder/internal#33

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extract RunCacheProbe function #284

extract RunCacheProbe function #284

johnstcn commented Jul 26, 2024 •

edited

Loading

johnstcn Jul 30, 2024

mtojek Jul 31, 2024

johnstcn Aug 1, 2024

dannykopping left a comment

dannykopping Jul 31, 2024

johnstcn Jul 31, 2024

dannykopping Jul 31, 2024

johnstcn Jul 31, 2024

dannykopping Jul 31, 2024

johnstcn Jul 31, 2024

mtojek Jul 31, 2024

mtojek left a comment

mtojek Jul 31, 2024

johnstcn Jul 31, 2024

mtojek Jul 31, 2024

mtojek Jul 31, 2024

johnstcn Jul 31, 2024

mtojek Jul 31, 2024

johnstcn Jul 31, 2024

mtojek Jul 31, 2024

mtojek Jul 31, 2024

johnstcn Jul 31, 2024

mtojek Jul 31, 2024

mtojek Jul 31, 2024

johnstcn Jul 31, 2024

mtojek Jul 31, 2024

johnstcn Jul 31, 2024

mtojek Jul 31, 2024

johnstcn Jul 31, 2024

mtojek Jul 31, 2024

extract RunCacheProbe function #284

extract RunCacheProbe function #284

Conversation

johnstcn commented Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannykopping 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

mtojek 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

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

johnstcn commented Jul 26, 2024 •

edited

Loading