-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
linter: add rule for relative path used in workdir #4903
Conversation
f9fde2f
to
041e58d
Compare
@@ -1124,6 +1126,8 @@ func dispatchRun(d *dispatchState, c *instructions.RunCommand, proxy *llb.ProxyE | |||
} | |||
|
|||
func dispatchWorkdir(d *dispatchState, c *instructions.WorkdirCommand, commit bool, opt *dispatchOpt) error { | |||
//validateWorkdir(c, d.image.Config.WorkingDir, d.platform.OS, opt.lintWarn) |
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.
sneaky comment
2ec81de
to
e8e0895
Compare
This is passing tests but I'm not sure if this implementation is the best way to go about this. There's something about the if statement for |
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'm going to think about it overnight and see if I like it more or less in the morning.
This approach SGTM
// checking the base image. | ||
if system.IsAbs(c.Path, d.platform.OS) { | ||
d.absWorkdirSet = true | ||
} else if !d.absWorkdirSet { |
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 showing this only on first relative WORKDIR
would be fine as that is the only line that would need to be fixed. So that means d.absWorkdirSet = true
in all cases.
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.
Considering this comment, I modified the code a bit to make it a bit more clear. It still uses commit
to determine whether to run the lint, but I've changed the variable to workdirSet
instead and had it unconditionally set to true. I think this is ready to be reviewed and merged if approved now.
e8e0895
to
ae6a1cb
Compare
ae6a1cb
to
a346644
Compare
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
a346644
to
218ce96
Compare
No description provided.