Skip to content
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

Merged
merged 1 commit into from
May 10, 2024

Conversation

jsternberg
Copy link
Collaborator

No description provided.

@jsternberg jsternberg force-pushed the lint-workdir-relative-path branch 2 times, most recently from f9fde2f to 041e58d Compare May 7, 2024 16:15
@jsternberg jsternberg marked this pull request as ready for review May 7, 2024 16:17
@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sneaky comment

@jsternberg jsternberg force-pushed the lint-workdir-relative-path branch 3 times, most recently from 2ec81de to e8e0895 Compare May 8, 2024 21:18
@jsternberg jsternberg marked this pull request as draft May 8, 2024 22:02
@jsternberg
Copy link
Collaborator Author

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 commit that I don't really like. I'm going to think about it overnight and see if I like it more or less in the morning.

Copy link
Member

@tonistiigi tonistiigi left a 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 {
Copy link
Member

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.

Copy link
Collaborator Author

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.

@jsternberg jsternberg marked this pull request as ready for review May 10, 2024 15:44
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
@tonistiigi tonistiigi merged commit 51feb60 into moby:master May 10, 2024
73 checks passed
@jsternberg jsternberg deleted the lint-workdir-relative-path branch May 13, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants