-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[context parser] Check current ref for Docker image existence #20345
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
Conversation
Thank you for the PR, @filiptronicek ! I do see the error now, but really wonder whether that is the right way to go about it: Ideally, I would see this "warning", but still be able to start he workspace with the default image... which is the behavior we had before, no? Or has that been changed by the recent changes around surfacing parsing errors? 🤔 |
@geropl I agree, we should not force folks to get stuck when they make a mistake. This check is helpful in that it allows us to tell the user at what actually is wrong. This was the previous state: |
@meysholdt reported this as a bug and as far as our current feature set goes, it is. It is not a hard error to fix even outside of Gitpod with web-native editors inside every SCM, but it would be great if supervisor could know about this condition and report it appropriately.
I'm going to put this back to draft and see what other options we have. Edit: it looks like supervisor does really only get the exit status, this will be tricky... |
forceCreateNewWorkspace?: boolean; | ||
forceImageBuild?: boolean; | ||
// The context can have non-blocking warnings that should be displayed to the user. | ||
warnings?: string[]; |
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.
🧡
if err != nil { | ||
log.WithError(err).Error("build failed") | ||
|
||
err := os.WriteFile("/workspace/.gitpod/bob.log", []byte(err.Error()), 0644) |
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.
🧡
} | ||
|
||
if (ExternalImageConfigFile.is(config.config.image)) { | ||
if (config.config.image.externalSource.revision === "") { |
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 assume here is the place I meant in https://github.com/gitpod-io/gitpod/pull/20345/files#r1832411675.
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.
Made it happen in fbd4b35
(#20345). I agree that it's better to have a special value.
Really like this change: Thoroughly thought through, fixed the underlying issue on all fronts we can consider, plus introducing a super-useful concept for context-parsing in general. 🧡 |
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.
Code LGTM tested and works as advertised! 🎉
Description
Whenever we try to parse the context for a workspace, we check that any Dockerfile mentioned in the
.gitpod.yml
'simage.file
actually exists and we also check when it was last modified.The problem is that for most providers, we did the existence check by listing commits for the given path, which gave false-positives for Dockerfiles that once existed but no longer do on the tip of the git reference.
In addition, we now don't let this error block workspace starts. Instead, we attach a warning to the workspace metadata and show it in the UI when starting.
Related Issue(s)
Follow-up for CLC-851 and #20267
How to test
You can try parsing https://github.com/spring-projects/spring-petclinic on the preview environment. It should let you create a workspace, but with a warning.