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

Logging bumped internal dependencies for packages included in a release is painful to do manually and is often forgotten #108

Open
mcmire opened this issue Nov 9, 2023 · 3 comments

Comments

@mcmire
Copy link
Contributor

mcmire commented Nov 9, 2023

This is an offshoot of #90.

When a new core release is created, we want consumers to be aware of any bumps to dependencies that occur in new versions of packages being included in that release. To do this, we ask that changelog entries be added for these dependency bumps.

At the same time, the core monorepo contains a Yarn constraint which enforces that when a workspace package is bumped, for every package which depends on that package, the version range for the dependency listed in that package's manifest must match the new version. Because release PRs involve bumping versions of packages within the monorepo, they are guaranteed to also contain dependency bumps for those packages. For instance, if @metamask/base-controller is bumped to 2.3.4, then any controller which depends on this package will be updated to use a version range of ^2.3.4 (it has to be).

When it comes to dependency bumps for packages that live outside of the monorepo — e.g. @metamask/rpc-errors — the author of a release PR is given a reminder to include any bumps related to those packages in changelogs. Because those bumps will have already occurred in commits prior to the release PR, create-release-branch (thanks to auto-changelog) will automatically pick up those commits and add them to the changelogs when it creates the release branch.

However, internal dependency bumps are another story. The command that adds them is yarn constraints --fix, which create-release-branch runs after creating the release branch; but because this does not occur within a prior commit, auto-changelog cannot see the bumps.

The result of this is the author of a release PR must check for and add these types of entries manually. This is not an obvious step, and more often than not, it is forgotten and these bumps go unlogged. We should prevent this from happening.

Acceptance Criteria

  • For each package-to-be-released, create-release-branch offers a way to detect bumps to dependencies that have occurred within the package and produce an error if those bumps are not reflected in changelog entries that correspond to the version-to-be-released for that package.
  • This check is added to the lint package script so that it can be run after creating a release branch to verify the changelog.
  • This check is also run in CI, so that the author is compelled to add missing entries if they don't exist. (This may happen naturally in the previous step.)
  • Optionally, create-release-branch offers a way to take a PR number that corresponds to the release branch and automatically add these changelog entries, saving time when creating a large release PR.

(Previous suggestion, preserved for posterity:)

We should consider adding a validate subcommand/flag to create-release-branch which would detect these kinds of dependency bumps relative to the latest release of the package that has them and ensure that entries are present in the package's changelog. We could even add a --fix flag which would automatically add the bumps.

I realize that this means that create-release-branch would no longer solely create a branch, it would also manage that branch, but perhaps we could rename the package at a later point in time.

@desi
Copy link
Contributor

desi commented Nov 9, 2023

@mcmire
Copy link
Contributor Author

mcmire commented Mar 26, 2024

Thinking about this some more, does it make sense to add this check to auto-changelog instead? We can investigate that just to be sure.

@cryptodev-2s cryptodev-2s self-assigned this Apr 9, 2024
@mcmire
Copy link
Contributor Author

mcmire commented May 13, 2024

In response to my previous comment, I think it makes the most sense to implement the validation described in this ticket in this repo rather than auto-changelog. auto-changelog only sees one package at a time, whereas create-release-branch has access to the entire monorepo. We may be able to add functionality to auto-changelog which makes the validation code shorter, but we can worry about that later.

Suggestions on how to implement this ticket:

  1. We use the yargs package to process command-line arguments (you can see the file that uses yargs here). This library gives us the ability to define a command, a word that places the tool in a different mode. We can re-implement the existing workflow as a default command and then implement validate as a subcommand by chaining more options onto yargs. See the yargs documentation on how to do this, particularly this section and this example in the yargs documentation.
  2. Once we are able to get create-release-branch to recognize a validate command, we can then implement it:
    1. Go through each workspace package and figure out which ones have version bumps. (We probably have this information already, because we make use of it to run the current set of validations in release-specification.ts.)
    2. Go through each workspace package again, and if there are any dependencies on other workspace packages that have been bumped (i.e. are present in the list created in step 1):
      1. Find the section of the changelog corresponding to the release we want to publish. This is something that auto-changelog already does, but we now have to do ourselves. This means that we need to make a new instance of the Changelog class, and calling methods on it to access the data within. See here on how to do this. (If this is not present, then we can throw an error for now.)
      2. Collect the changelog entries in this section and look for an entry that records the dependency bump (perhaps "Bump ${package} to ${new_version}" or "Bump ${package} from ${old_version} to ${new_version}"). If this entry is not present, then record the validation error in an array and move on to the next workspace package.
    3. If the list of validation errors collected in step 2 is non-empty, then use it to print some kind of report and then exit with code 1 (or we can just throw an error, whichever is easier).

I mentioned in the ticket description that we may want to implement a --fix option. Here are some suggestions on how to do that:

  1. We can configure yargs to recognize this option as well. We already configure yargs to look for some options, so you can take a look at command-line-arguments.ts for examples — we'll just want to make sure we define those options on the validate command and not the default command.
  2. The --fix option should perform the same steps as in non-fix mode, except that instead of recording validation errors when it sees that a changelog entry is missing, it should just go ahead and add it. Within the section of a changelog that corresponds to the new release, the entry for a package bump should go in the "Changed" sub-section. If this section does not exist, then we should create it. We can do this easily using the Changelog class in auto-changelog: scan through the methods there and see if you can find one that makes this possible.

Make sure to update the functional tests in functional.test.ts as you implement this behavior. We may even want to have two files, one for the default command, the other for the validate command.

@mcmire mcmire changed the title Add a validation step which confirms all changelog entries are present for workspace packages bumped within a release PR Logging bumped internal dependencies for packages-to-be-released is painful to do manually Jun 6, 2024
@mcmire mcmire changed the title Logging bumped internal dependencies for packages-to-be-released is painful to do manually Logging bumped internal dependencies for packages included in a release is painful to do manually Jun 6, 2024
@mcmire mcmire changed the title Logging bumped internal dependencies for packages included in a release is painful to do manually Logging bumped internal dependencies for packages included in a release is painful to do manually and is often forgotten Jun 6, 2024
@cryptodev-2s cryptodev-2s removed their assignment Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants