| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Review requested:
|
Sorry, something went wrong.
|
Im not sure about updating the gyp file, its too complex to automate it we should probably do it manually |
Sorry, something went wrong.
I tend to agree with that in general. However, in this case, if the script does make a mistake in producing the gyp file, we still have our regular review process as well as Github Actions and Jenkins CI as safeguards, and we can still fix the gyp file manually if necessary. A mistake that is not caught by any of these safeguards could just as well be the result of manual work. In other words, we can take the generated gyp file as a suggestion, which hopefully will be correct in most cases, thus usually removing the need to update it manually. When we or our automated tests notice a problem with the suggested change, we can still resort to updating it manually. I'll try and see what happens with previous versions of googletest (i.e., if the script would have made mistakes in the past). That being said, I'd be happy to switch to a more reliable approach if we can come up with one. |
Sorry, something went wrong.
There was a problem hiding this comment.
I like this approach. Recently, I read about a similar blog post about how Github updates its Ruby version every week by following main branch. We can always revert back or interfere with the upgrade process and I believe this would be a valuable lesson/experience we could benefit as the whole Node.js project.
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM, lets try with this approach if we are happy we can extend it to zlib and also try to automate some other dependencies gyp
Sorry, something went wrong.
|
Appeased lint-sh. PTAL @marco-ippolito. |
Sorry, something went wrong.
|
Rebased again and updated for #47591. |
Sorry, something went wrong.
GoogleTest follows the Abseil Live at Head philosophy, and rarely creates tags or GitHub releases, so instead, follow Google's recommendation and update to the upstream HEAD every once in a while. The tricky bit is properly updating googletest.gyp, and this script might fail doing so in the future. Refs: nodejs/security-wg#828
|
@marco-ippolito Is my understanding correct that the GitHub action will keep updating the same PR instead of opening new ones if a new update is found before the PR is merged? That would be great to reduce noise in this case, if we could just keep the PR open regardless of how often the action runs. |
Sorry, something went wrong.
|
if there is a pr for 1.0.1 and the version1.0.2 is released, another pr will be created. if the latest is the 1.0.1 pr it will not create another one for every run. I dont think it doesnt happen very often but it would be great to update or close the PR for the previous version. |
Sorry, something went wrong.
According to https://github.com/gr2m/create-or-update-pull-request-action#how-it-works it should update the existing PR (more specifically, it pushes to the same branch as the previously opened PR for the dependency). I've seen this happen on some of the Undici PRs (e.g. #46502). |
Sorry, something went wrong.
Oh that's great then we are good on this |
Sorry, something went wrong.
|
I like that not creating additional PRs reduces notification noise, but it unfortunately seems to unnecessarily keep old commits and also doesn't update the PR title or description. |
Sorry, something went wrong.
Updating the title/body can be controlled by the update-pull-request-title-and-body action parameter. |
Sorry, something went wrong.
|
Thanks, I saw that in the docs right after I left the previous comment and opened #47621. Regardless, this PR needs to be re-approved after the last force-push :) |
Sorry, something went wrong.
GoogleTest follows the Abseil Live at Head philosophy, and rarely creates tags or GitHub releases, so instead, follow Google's recommendation and update to the upstream HEAD every once in a while. The tricky bit is properly updating googletest.gyp, and this script might fail doing so in the future. Refs: nodejs/security-wg#828 PR-URL: #47482 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
GoogleTest follows the Abseil Live at Head philosophy, and rarely creates tags or GitHub releases, so instead, follow Google's recommendation and update to the upstream HEAD every once in a while. The tricky bit is properly updating googletest.gyp, and this script might fail doing so in the future. Refs: nodejs/security-wg#828 PR-URL: #47482 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
GoogleTest follows the Abseil Live at Head philosophy, and rarely creates tags or GitHub releases, so instead, follow Google's recommendation and update to the upstream HEAD every once in a while. The tricky bit is properly updating googletest.gyp, and this script might fail doing so in the future. Refs: nodejs/security-wg#828 PR-URL: nodejs#47482 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
| Back | FazBrowse Home | New Git URL |
I haven't done one of these before, so please review carefully.
GoogleTest follows the Abseil Live at Head philosophy, and rarely creates tags or GitHub releases, so instead, follow Google's recommendation and update to the upstream HEAD every once in a while.
The tricky bit is properly updating googletest.gyp, and this script might fail doing so in the future.
Refs: nodejs/security-wg#828