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

move back to hard-coded modes if mode is not provided #11760

Merged
merged 5 commits into from
Mar 11, 2025

Conversation

jakecoffman
Copy link
Member

@jakecoffman jakecoffman commented Mar 7, 2025

What are you trying to accomplish?

The PR above was meant to fix #6081 but the one report of it being solved later was rescinded.

We're also seeing in the data an invalid file mode. Git uses a restricted set of file modes and in order to get the correct mode we'd need to commit a file and run git ls-tree HEAD -- <file> on it.

However, we can't assume that initializing a DependencyFile means there's an actual repo at that path at that time. This means we can't run that command inside DependencyFile, the mode needs to be passed in. So I've left the mode parameter which will allow us in the future to override the mode to make a file executable when needed.

Anything you want to highlight for special attention from reviewers?

How will you know you've accomplished your goal?

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@jakecoffman
Copy link
Member Author

Composer and NPM seem to be having issues so I won't be able to merge, but I can fix up the handful of smoke tests this broke. Interestingly most of the smoke tests do no have modes set. Just bundler (where we're seeing an issue) and Go.

@jakecoffman jakecoffman marked this pull request as ready for review March 7, 2025 17:37
@jakecoffman jakecoffman requested a review from a team as a code owner March 7, 2025 17:37
@jakecoffman jakecoffman requested a review from pavera March 7, 2025 20:56
Comment on lines +94 to +95
@mode = mode
raise ArgumentError, "Invalid Git mode: #{mode}" if mode && !VALID_MODES.include?(mode)
Copy link
Member

Choose a reason for hiding this comment

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

For my understanding, can you share why nil is something we should allow? I understand that it is a value that we currently handle.

Copy link
Member Author

Choose a reason for hiding this comment

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

Prior to #6141 mode was always nil... well it wasn't even a field. We relied on the PullRequestCreator to set the mode based on the type.

So now I am returning to that mostly, except it does seem good to allow for overriding this value to fix the executable problem per ecosystem. However adding an error on bad values immediately if one is provided seems like another good thing to do.

Copy link
Contributor

@pavera pavera left a comment

Choose a reason for hiding this comment

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

I can't quite remember with precision how this works, though I know I did spend some time with this a while ago (1.5 years?). If this works, LGTM :).

@jakecoffman jakecoffman merged commit 4ded781 into main Mar 11, 2025
126 of 134 checks passed
@jakecoffman jakecoffman deleted the use-hardcoded-modes branch March 11, 2025 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Yarn v2/v3 zero-install file permissions are changed
3 participants