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

feat: add additional-paths property to manifest releaser #2336

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

Conversation

jack-lewin
Copy link

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #1921 🦕


This PR adds an additional-paths property to the manifest releaser.

Additional paths are useful for monorepos where a package might depend files from other directories.

For example, if we want to trigger a release in apps/my-app-2 when commits are found in libs/my-lib:

apps/
    my-app-1
    my-app-2
libs/
    my-lib
{
  "packages": {
    "apps/my-app-1": {},
    "apps/my-app-2": {
      "additional-paths": ["libs/my-lib"] // additional-paths would be a new config option
    }
  }
}

Best reviewed commit-by-commit.

Before, packagePaths was an array of strings, i.e. the name of each package.

Now, packagePaths is a Record<string, string[]>, with the key being the name of each package,
and the value being an aray of additionalPaths for that package.
@jack-lewin jack-lewin requested review from a team as code owners July 13, 2024 13:37
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Jul 13, 2024
@jack-lewin
Copy link
Author

Hi @chingor13, sorry for the nudge - are you still maintaining this project?
If not, is there anyone else I should reach out to?

Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Thanks for this and sorry for the delay in reviewing.

This mostly looks fine although we should explicitly define what the expected logic is if an additional path coincides with a different component's path or additional paths. Currently, we sort by the components' path length with the longest first as a way to prefer the deepest path in the case of overlapping paths. With this new logic, we are still sorting based on the main path, and preferring its additional paths over any other ones.

Instead, we may need to invert the mapping and have the keys be all the "paths" (main & additional) and the value is the main component path to associate to.

@sylwit
Copy link

sylwit commented Sep 30, 2024

Hi @jack-lewin will you have time to update your PR or do you need help ? Thank you for this PR

@roivanov
Copy link

roivanov commented Oct 4, 2024

sync with the latest main: jack-lewin#1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to define multiple paths
4 participants