-
Notifications
You must be signed in to change notification settings - Fork 335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
POD-779 | Initial integration of devpod-crane #1230
POD-779 | Initial integration of devpod-crane #1230
Conversation
ef201c2
to
580d6df
Compare
580d6df
to
91dd848
Compare
91dd848
to
18d85e6
Compare
|
||
GitCrane = "git" | ||
|
||
BinPath = "devpod-crane" // FIXME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this comment still valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah would be nice to make it at least somehow configurable to support dev/debug binaries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I really like some of the refactoring done here and the new code looks good. I just had a couple of comments / questions then ready to merge
} | ||
|
||
if language.MapConfig[lang] == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to check the key exits first
if _, ok := language.MapConfig[lang]; ok {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah definitely. didn't really go into this code yet, tried to break it down a bit to make it easier to refactor.
pkg/devcontainer/run.go
Outdated
return options.Recreate && !isDockerDriver | ||
} | ||
|
||
func (r *runner) getRawConfig(options provider2.CLIOptions) (*config.DevContainerConfig, error) { | ||
if r.WorkspaceConfig.Workspace.DevContainerConfig != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this if else if else if block is a little hard to read. Since we have early returns in each block can we separate into separate if statements?
pkg/devcontainer/run.go
Outdated
// will be gracefully handled by the auto-detection mechanism | ||
if err != nil && !os.IsNotExist(err) { | ||
return nil, errors.Wrap(err, "parsing devcontainer.json") | ||
} else if rawParsedConfig == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else if can be replaced here with separate if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Part of POD-779