Runtime refactor #3348
Runtime refactor #3348
Conversation
There was a problem hiding this comment.
With a PR this big it's easy to get lost in the weeds. I tried to take a step back and look at how the new code is structured and how it is used throughout our codebase. From that standpoint this PR is a huge success. The new runtime code is easy to read and understand and it's uses all make sense to me and are an improvement over what we had before. Great job!
There are a few more minor comments and things to consider before we land this.
| if err != nil { | ||
| wp.errorsOccurred = true | ||
| } | ||
| wp.errors <- err |
There was a problem hiding this comment.
Is it possible that we are writing nil to the error channel for every invocation of fn? If we move this inside the err != nil block can be omit the continue on line 80?
| @@ -81,3 +81,7 @@ func IsNetworkingError(err error) bool { | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
| func RuntimeDisabled() bool { | |||
| return os.Getenv(constants.DisableRuntime) == "true" | |||
There was a problem hiding this comment.
May want to use strings.EqualFold here
There was a problem hiding this comment.
Gonna leave it as it's inconsistent with the logic of the rest of that file. Making these case insensitive doesn't seem to have much value imo.
internal/osutils/exeutils.go
Outdated
| func ExecutablePaths(env map[string]string) ([]string, error) { | ||
| // Retrieve artifact binary directory | ||
| var bins []string | ||
| if p, ok := env["PATH"]; ok { |
There was a problem hiding this comment.
nit: can return early here if PATH isn't set.
| @@ -30,7 +30,7 @@ func (rt *RoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { | |||
|
|
|||
| resp, err := rt.client.Do(retryableReq) | |||
| if err != nil && resp != nil && resp.StatusCode == http.StatusForbidden && strings.EqualFold(resp.Header.Get("server"), "cloudfront") { | |||
| return nil, platform.NewCountryBlockedError() | |||
| return nil, api_errors.NewCountryBlockedError() | |||
There was a problem hiding this comment.
There is no right or wrong here but I'm only used to seeing underscores for test and build files. I believe we've avoided snake case for package names and other tokens in Go.
There was a problem hiding this comment.
Yeah, a while ago I had brought this up in stand, but it probably bears repeating. Our codebase has gotten to such a large size that following idiomatic Go naming conventions gives us too many collisions. At which point we're using aliases where needed, and those aren't usually very consistent.
Given the givens I'd rather have underscores and non-colliding package names than no underscores and collision galore.
Imo the naming conventions only make sense on projects that are Go libraries, once you start working on a Go application it very quickly stops making sense.
There was a problem hiding this comment.
I agree that the aliases were getting a bit out of hand but I'm still not seeing how underscores solve this problem. Could api_errors not be apiErrors or does go complain about that as well?
Either way this should be considered a nit as I don't want to hold the PR up. I just wanted to flag us introducing something that we have called out in the past for both file and package names.
There was a problem hiding this comment.
Oh sure, yeah it could be. Especially now that we're ignoring idiomatic Go in this context anyway. I just used underscore since that at least partly aligns with Go's standard (eg. pkg_test.go), plus it sidesteps the whole case sensitive filesystems issue.
| if a, ok := p.buildsExpected[id]; ok { | ||
| name = a.NameAndVersion() | ||
| } | ||
| case StepDownload: |
There was a problem hiding this comment.
Should this also cover StepUnpack?
internal/runners/refresh/refresh.go
Outdated
| return locale.NewInputError("refresh_runtime_uptodate") | ||
| } | ||
|
|
||
| r.prime.SetProject(proj) |
There was a problem hiding this comment.
I don't believe proj is changing between here and where SetProject is called on line 73. Is this call necessary?
| if err := os.MkdirAll(dest, 0755); err != nil { | ||
| return errs.Wrap(err, "could not create directory %s", dest) | ||
| } | ||
| entries, err := os.ReadDir(src) | ||
| if err != nil { | ||
| return errs.Wrap(err, "could not read directory %s", src) | ||
| } | ||
| for _, entry := range entries { | ||
| if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name())); err != nil { | ||
| return errs.Wrap(err, "sub link failed") | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
nit: could be linkDir function similar to linkFile below.
There was a problem hiding this comment.
There is no linkDir function? That's basically what Link() is.
| if err := linkFile(src, dest); err != nil { | ||
| return errs.Wrap(err, "could not link %s to %s", src, dest) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
nit:
| if err := linkFile(src, dest); err != nil { | |
| return errs.Wrap(err, "could not link %s to %s", src, dest) | |
| } | |
| return nil | |
| return linkFile(src, dest) |
But then you may have to adjust the error in the caller.
There was a problem hiding this comment.
That's an anti-pattern; we would loose the context of where linkFile encountered an error. Of course it's in this case pretty self-explanatory, but we don't know how this code might evolve.
pkg/buildplan/buildplan.go
Outdated
| result = strfmt.UUID(id.ID) | ||
| } | ||
| return result, nil | ||
| func (b *BuildPlan) RecipeID() strfmt.UUID { |
There was a problem hiding this comment.
Perhaps this function should also be called LegacyRecipeID
pkg/buildplan/filters.go
Outdated
| func FilterNeedsBuild() FilterArtifact { | ||
| return func(a *Artifact) bool { | ||
| return a.Status != types.ArtifactSucceeded |
There was a problem hiding this comment.
Is this true? A failed or skipped artifact does not need to be built and it won't be in the future.
There was a problem hiding this comment.
I've renamed it to FilterNotBuild(), because the code using this does in fact want artifacts that failed.
# Conflicts: # internal/runbits/runtime/refresh.go # internal/runbits/runtime/requirements/requirements.go # internal/runners/checkout/checkout.go # internal/runners/commit/commit.go # internal/runners/exec/exec.go # internal/runners/initialize/init.go # internal/runners/packages/import.go # pkg/executors/executors.go
No description provided.