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

Remove dependency on uuid package #1824

Merged
merged 1 commit into from
Oct 2, 2024
Merged

Conversation

joshmgross
Copy link
Member

A common pattern in the toolkit is to create temporary file or directory within the existing temp directory using a random UUID.

Versions of the uuid package below v7 are deprecated, so we shouldn't depend on them. Additionally, Node has a built-in crypto.randomUUID() method that can be used to generate UUIDs without the need for an external package. This function was introduced in versions of Node 14 and 15, so we should be safe to use it for actions that depend on both Node 16 and 20 (which are our only supported versions at this time).

I also went ahead and updated @actions/core, even though it was using a non-deprecated version of the uuid package.

Since these use cases is purely for temporary files, I don't think we strictly need a cryptographically secure UUID but that's an added bonus of using the built-in Node function.

Packages shouldn't be depending on the exact format of this temp file/directory, but even if they were this should be a compatible as we're still generating a version 4 UUID

Generates a random RFC 4122 version 4 UUID. The UUID is generated using a cryptographic pseudorandom number generator.

There are a couple other packages within the toolkit that have an indirect dependency on uuid through @actions/core, those can be updated as well once we release this new version of @actions/core.

@joshmgross joshmgross requested review from a team as code owners September 4, 2024 18:36
@MikeMcC399
Copy link

@joshmgross

Are you going to be able to progress this PR soon? It looks like you are just waiting for your team to review.

@joshmgross joshmgross merged commit 78af634 into main Oct 2, 2024
14 checks passed
@joshmgross joshmgross deleted the joshmgross/rm-uuid-dep branch October 2, 2024 17:49
zemnmez-renovate-bot added a commit to zemn-me/monorepo that referenced this pull request Oct 2, 2024
github-merge-queue bot pushed a commit to zemn-me/monorepo that referenced this pull request Oct 2, 2024
@gfteix
Copy link

gfteix commented Oct 2, 2024

As the crypto global package is being used without an import/require statement, if the client is using a node version < 19 (when crypto became a global package in node.js https://nodejs.org/api/globals.html#crypto_1) the action that uses the toolkit will fail with a crypto is not defined error. Is this expected?

@MarioUhrikTakeda
Copy link

This seems to have caused #1841
FYI @joshmgross

@MikeMcC399
Copy link

MikeMcC399 commented Oct 3, 2024

@joshmgross

Just to be sure, are you (or somebody else) going to release a new version of @actions/cache now?

Edit: removed misleading screenshot of @actions/cache@3.2.4

@joshmgross
Copy link
Member Author

@MikeMcC399 yes I'm planning to upgrade the packages that depend on @actions/core.

It's worth noting that @actions/cache will still depend on uuid through @azure/core-http though:

❯ npm why uuid
uuid@8.3.2
node_modules/@azure/core-http/node_modules/uuid
  uuid@"^8.3.0" from @azure/core-http@3.0.2
  node_modules/@azure/core-http
    @azure/core-http@"^3.0.0" from @azure/storage-blob@12.15.0
    node_modules/@azure/storage-blob
      @azure/storage-blob@"^12.13.0" from the root project

uuid@8.3.2
node_modules/@azure/ms-rest-js/node_modules/uuid
  uuid@"^8.3.2" from @azure/ms-rest-js@2.7.0
  node_modules/@azure/ms-rest-js
    @azure/ms-rest-js@"^2.6.0" from the root project

@MikeMcC399
Copy link

@joshmgross

yes I'm planning to upgrade the packages that depend on @actions/core.

It's worth noting that @actions/cache will still depend on uuid through @azure/core-http though:

Thanks for the confirmation! Looking forward to new releases which no longer depend on a deprecated version of uuid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants