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

Fix Nuget grouped PR's #9228

Merged
merged 6 commits into from
Mar 7, 2024
Merged

Conversation

sebasgomez238
Copy link
Contributor

@sebasgomez238 sebasgomez238 commented Mar 6, 2024

Issue occured because we were resetting repo files when we shouldn't. Using this helper method allows us to reset only when appropriate.

@sebasgomez238 sebasgomez238 requested a review from a team as a code owner March 6, 2024 19:23
@github-actions github-actions bot added the L: dotnet:nuget NuGet packages via nuget or dotnet label Mar 6, 2024
@sebasgomez238 sebasgomez238 marked this pull request as draft March 6, 2024 19:29
@jakecoffman
Copy link
Member

During a grouped update, the updater code will keep track of the modified dependency files in-memory and hand them off to the FileUpdater for each iteration of the dependencies:

# Get the current state of the dependency files for use in this iteration, filter by directory
dependency_files = group_changes.current_dependency_files(job)

So I suspect the problem is not that the directory is getting reset each iteration, but the modified dependency files need to be written to disk before the NuGet native updater is used.

Does that make sense?

@sebasgomez238
Copy link
Contributor Author

sebasgomez238 commented Mar 6, 2024

@jakecoffman The behavior I observed when removing the line that resets the changes was that a single grouped update would work as expected but if multiple individual updates were applied in a single job then the updates would become cumulative where in the end the last update contained all of the previous updates.

Wouldn't this mean that the modified dependency files are already being written to the disk? (Please correct me if I'm wrong)

In any case, a working solution would be to have the updater only reset if we're not conducting a grouped update which brings a lot of edge cases such as handling multiple groups or a group + an individual update. Let me know if you feel like we should approach this differently.

@jakecoffman
Copy link
Member

a working solution would be to have the updater only reset if we're not conducting a grouped update

We already have that with this helper 👇

def self.in_a_temporary_repo_directory(directory = "/", repo_contents_path = nil, &block)
if repo_contents_path
# If a workspace has been defined to allow orcestration of the git repo
# by the runtime we should defer to it, otherwise we prepare the folder
# for direct use and yield.
if Dependabot::Workspace.active_workspace
T.must(Dependabot::Workspace.active_workspace).change(&block)
else
path = Pathname.new(File.join(repo_contents_path, directory)).expand_path
reset_git_repo(repo_contents_path)
# Handle missing directories by creating an empty one and relying on the
# file fetcher to raise a DependencyFileNotFound error
FileUtils.mkdir_p(path)
Dir.chdir(path) { yield(path) }
end
else
in_a_temporary_directory(directory, &block)
end
end

For non-grouped updates it goes into this path:

path = Pathname.new(File.join(repo_contents_path, directory)).expand_path
reset_git_repo(repo_contents_path)
# Handle missing directories by creating an empty one and relying on the
# file fetcher to raise a DependencyFileNotFound error
FileUtils.mkdir_p(path)
Dir.chdir(path) { yield(path) }

For a grouped update it defers to the active workspace, where it resets at the end of all updates:

Here's probably the clearest example of how to use it:

def updated_yarn_lock(yarn_lock)
base_dir = dependency_files.first.directory
SharedHelpers.in_a_temporary_repo_directory(base_dir, repo_contents_path) do
write_temporary_dependency_files(yarn_lock)
lockfile_name = Pathname.new(yarn_lock.name).basename.to_s
path = Pathname.new(yarn_lock.name).dirname.to_s
updated_files = run_current_yarn_update(
path: path,
yarn_lock: yarn_lock
)
updated_files.fetch(lockfile_name)
end
rescue SharedHelpers::HelperSubprocessFailed => e
handle_yarn_lock_updater_error(e, yarn_lock)
end

write_temporary_dependency_files is where it takes the in-memory files and writes them to the repo directory which may or may not have been reset, depending on if this is a grouped update or not.

@sebasgomez238 sebasgomez238 marked this pull request as ready for review March 7, 2024 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: dotnet:nuget NuGet packages via nuget or dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants