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

Building commit file path from repo root instead of execution dir #2456

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Maxim-Durand
Copy link

@Maxim-Durand Maxim-Durand commented Apr 6, 2024

What Changed

Modified packages/core/src/git.ts so that getGitLog returns a list of commit object with the modified files path resolved from the root of the git repository instead of the execution directory.

Why

Commits objects were built with

files: (commit.files || []).map((file) => path.resolve(file)),

But the documentation of path.resolve states:

If, after processing all given path segments, an absolute path has not yet been generated, the current working directory is used.

Since it was only passing the file path returned by git log (which is a relative path) there was no absolute path generated and it would hence append the current directory (i.e the directory from where auto is run from) as a prefix to what git log returned. Meaning this would produce the correct absolute path only if auto is run from the root of the repository.

Todo:

  • Add tests
  • Add docs

Change Type

Indicate the type of change your pull request is:

  • documentation
  • patch
  • minor
  • major

@Maxim-Durand Maxim-Durand changed the title Auto appending commit file path from repo root instead of execution dir Building commit file path from repo root instead of execution dir Apr 6, 2024
@hipstersmoothie hipstersmoothie added the patch Increment the patch version when merged label Apr 10, 2024
@Maxim-Durand
Copy link
Author

@hipstersmoothie Can you restart the CI ?

@Maxim-Durand
Copy link
Author

The CI failures seem unrelated to my code changes but not sure. @hipstersmoothie Should I investigate more or is this a known issue ?

@Maxim-Durand
Copy link
Author

@hipstersmoothie I'm really sorry for the spam but I'm also really eager to see this bug fix released so I'm able to use auto with https://github.com/restfulhead/npm-auto-plugins/tree/main/packages/filter-by-workspace-path

Considering the very small change involved in this PR and the fact CI pipelines seem broken, would you consider accepting the risk of merging without a green CI to release this bug fix sooner rather than later ?

On a different note, what is the process to add/reference a new plugin ? Does it have to be hosted in the plugins folder or can I just make a PR to change the readme and reference https://github.com/restfulhead/npm-auto-plugins/tree/main/packages/filter-by-workspace-path ?

@hipstersmoothie
Copy link
Collaborator

@Maxim-Durand maybe it's the node version in the actions that's causing the tests to fail? we need those passing before a merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants