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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci(workflows): bump 'setup-node' to v2 and use 'cache' option #44924

Closed
wants to merge 1 commit into from
Closed

ci(workflows): bump 'setup-node' to v2 and use 'cache' option #44924

wants to merge 1 commit into from

Conversation

oscard0m
Copy link

@oscard0m oscard0m commented Jul 7, 2021

Description

  • Add cache to workflows using actions/setup-node
  • Bump actions/setup-node to v2 to give support to this cache option

Context

setup-node GitHub Action just released a new option to add cache to steps using it.

You can find the details here: https://github.blog/changelog/2021-07-02-github-actions-setup-node-now-supports-dependency-caching/

Fix #44898


馃 This PR has been generated automatically by this octoherd script, feel free to run it in your GitHub user/org repositories! 馃挭馃従


Migrated from #44897 due to indentation changes problem (GitHub was still showing indentation changes even I was restoring back the original indentation)

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jul 7, 2021
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@oscard0m
Copy link
Author

oscard0m commented Jul 7, 2021

First - it'd be super nice if you could split the formatting differences into a separate PR - I can't tell if the whitespace changes are good or bad.

馃憤馃徑 Ok to remove indentation.

Second - the GH actions build is currently a bit simpler than the DevOps one because it lacked caching - while caching most dependencies is fine, tslint (probably eslint and ts-related plugins nowadays) and typescript in the tree need to be cache-busted explicitly, so we always get the latest nightly of them. The DevOps build (which has caching) accomplishes this by explicitly uninstalling and reinstalling those dependencies after the initial (cached) install. I'm not sure if the node setup action's caching strategy is amenable to this, but we'd want it to do the same if it caches (since if we stop building against nightly tslint/eslint/typescript, we're liable to break the combination of them and not know when).

If I'm understanding correctly:

  • ci.yml and release-branch-artifact.yml are the ones with this npm uninstall use case. When you say build and DevOps you refer to these ones?

What I suggest to do is:

  • We can open a different issue and do the research to see if setup-node is supporting this "selective cache" (so we can apply it for typescript and tslint).
  • To move forward with the rest of the GitHub Actions with this change and rollback the cache option for ci.yml and release-branch-artifact.yml
    • 鈿狅笍 A new change I had to apply is to bump setup-node to v2 to give support to this cache option

Let me know what do you think @weswigham and @RyanCavanaugh

@sandersn sandersn added this to Not started in PR Backlog Jul 15, 2021
@sandersn sandersn requested a review from weswigham August 3, 2021 14:22
@sandersn
Copy link
Member

sandersn commented Aug 3, 2021

@weswigham you reviewed #44897; this is the followup to that.

@sandersn sandersn moved this from Not started to Waiting on reviewers in PR Backlog Aug 3, 2021
@sandersn sandersn added the Housekeeping Housekeeping PRs label Aug 3, 2021
@weswigham
Copy link
Member

My comment from the other PR - about needing cache-busting for certain packages that we pin to latest like in the DevOps builds - still stands.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Requesting changes to make the current status of the PR explicit.

PR Backlog automation moved this from Waiting on reviewers to Waiting on author Aug 6, 2021
@sandersn
Copy link
Member

@oscard0m is @weswigham's request feasible for you to do? I don't think we'll able to take this PR without it. =(

@oscard0m
Copy link
Author

From what I've read in setup/node ADR the cache option only covers the basic use-case of caching all the dependencies. For "more sophisticated" setups like the one of non-caching some dependencies should be covered combining actions/cache (and I'm not sure if that would be possible).

I just created an issue to actions/setup-node to ask more details on what is implemented by this cache option and actions/cache to see if there is a way to partially cache dependencies: actions/setup-node#321


@weswigham
I did not get clarified my questions here so I'm not sure if my changes would have sense to some of the workflows or they don't have sense for none of them (in case there is no easy solution with actions/cache or actions/setup-node).

@oscard0m
Copy link
Author

oscard0m commented Aug 23, 2021

Issue replied by a contributor: actions/setup-node#321 (comment)
There is no way today to do partial caching with actions/setup-node or actions/cache so I guess we can undo these changes for the GitHub Actions doing this uninstall step.

After a bit of research on npm existing solutions for cleaning the cache I discovered npm@7.21.0 added support for npm cache rm <key>. I would say we can keep On Hold this part until npm 7 becomes the default version as I suggested in the original issue discussion.

I keep waiting for your feedback regarding the other actions not uninstalling packages as a step. :)

@oscard0m oscard0m requested a review from sandersn August 24, 2021 18:59
Copy link

@vinayakkulkarni vinayakkulkarni left a comment

Choose a reason for hiding this comment

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

LGTM

@weswigham
Copy link
Member

Just gunna close this and someone can come forward with another version that does cache busting where needed. Or not; this isn't exactly a high priority thing.

@weswigham weswigham closed this Mar 4, 2022
PR Backlog automation moved this from Waiting on author to Done Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug Housekeeping Housekeeping PRs
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

To add cache support for GitHub Actions using setup-node
5 participants