Skip to content
This repository has been archived by the owner on Jan 13, 2024. It is now read-only.

chore: add ci, stale workflows and dependabot #1074

Merged
merged 1 commit into from
Mar 24, 2021
Merged

chore: add ci, stale workflows and dependabot #1074

merged 1 commit into from
Mar 24, 2021

Conversation

robertsLando
Copy link
Contributor

No description provided.

@robertsLando robertsLando requested a review from jesec March 23, 2021 08:57
@robertsLando
Copy link
Contributor Author

@leerob @jesec Seems all tests work except on windows. Each test takes ~30' on linux and ~50' on macos. We should find a way to speed up them at least to less then 10 minutes

@jesec
Copy link
Contributor

jesec commented Mar 23, 2021

@leerob @jesec Seems all tests work except on windows. Each test takes ~30' on linux and ~50' on macos. We should find a way to speed up them at least to less then 10 minutes

Indeed. We can split tests into different groups, so we can run them on multiple machines in parallel. Or can we simply run multiple tests in parallel on one machine, as pkg seems to be a single-thread task?

@robertsLando
Copy link
Contributor Author

We could use nodejs cluster module for that I think?

@leerob
Copy link
Member

leerob commented Mar 23, 2021

Let's combine efforts here #1073

@robertsLando
Copy link
Contributor Author

Let's combine efforts here #1073

@leerob This adds much more and it's more complete than that

.github/dependabot.yml Show resolved Hide resolved
.github/workflows/close-stale.yml Show resolved Hide resolved
@@ -1,5 +1,8 @@
{
"parserOptions": {
"sourceType": "script"
},
"rules":{
"linebreak-style": 0
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a very important rule. It makes sure that people don't accidentally use CRLF.

Can we fix the Windows problem with other methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried another approach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed lint from the test command and I check linting only on one os/node version (useless to do linting on all platforms/node version)

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@leerob
Copy link
Member

leerob commented Mar 23, 2021

@leerob This adds much more and it's more complete than that

I'm fine with whatever, just mentioning we should combine efforts into a single PR!

@robertsLando
Copy link
Contributor Author

@leerob @jesec Seems that tests are failing on windows, I would remove it for now and also remove macos and just keep ubuntu like it was with trevis

@robertsLando
Copy link
Contributor Author

robertsLando commented Mar 23, 2021

I also tried writing a script like this to speed up tests:

'use strict';

const cluster = require('cluster');
const { spawn } = require('child_process');
const path = require('path');
const os = require('os');

const args = [
    [ 'node14', 'no-npm' ],
    [ 'node12', 'no-npm' ],
    [ 'node10', 'no-npm' ],
    [ 'host', 'only-npm' ]
];

if (cluster.isMaster) {
    console.log('Parallel tests starting');

    // Fork workers.
    for (const a of args) {
        cluster.fork({ args: a.join(' ') });
    }

    cluster.on('exit', (worker) => {
        console.log(`worker ${worker.process.pid} died`);
    });
} else {
    const cache = path.join(os.homedir(), '.pkg-cache', process.env.args);
    spawn('node', [ './test/test.js', ...process.env.args.split(' ') ], {
        cwd: process.cwd(),
        env: {
            ...process.env,
            PKG_CACHE_PATH: cache
        },
        detached: true,
        stdio: 'inherit'
    });
}

The problem is I don't think some of them can be run in parallel as I'm getting different errors

@robertsLando
Copy link
Contributor Author

@leerob @jesec Updates here?

@@ -2,4 +2,4 @@
"parserOptions": {
"sourceType": "script"
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary EOF change here.

@jesec
Copy link
Contributor

jesec commented Mar 24, 2021

There is a minor EOF issue. And, we probably don’t want to run dependabot daily. Other than that, lg2m.

@leerob
Copy link
Member

leerob commented Mar 24, 2021

LGTM

@jesec
Copy link
Contributor

jesec commented Mar 24, 2021

Rebased, squashed, and made minor changes, per prior discussions:

diff --git a/.github/dependabot.yml b/.github/dependabot.yml
index 3509525..b339b63 100644
--- a/.github/dependabot.yml
+++ b/.github/dependabot.yml
@@ -3,7 +3,7 @@ updates:
   - package-ecosystem: npm
     directory: '/'
     schedule:
-      interval: daily
+      interval: 'weekly'
     allow:
       - dependency-type: 'production'
     labels:
@@ -12,7 +12,7 @@ updates:
   - package-ecosystem: 'npm'
     directory: '/'
     schedule:
-      interval: 'daily'
+      interval: 'weekly'
     target-branch: master
     allow:
       - dependency-type: 'development'
@@ -23,4 +23,4 @@ updates:
   - package-ecosystem: 'github-actions'
     directory: '/'
     schedule:
-      interval: 'daily'
+      interval: 'weekly'
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 23d8a18..ad791c3 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -1,4 +1,4 @@
-name: ci
+name: CI
 
 on:
   push:
diff --git a/.gitignore b/.gitignore
index ccea779..62ffed4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -17,4 +17,5 @@ examples/express/express-example
 # Example dependencies
 examples/express/node_modules
 
-/.vscode/
+# Editors
+.vscode/
diff --git a/README.md b/README.md
index 306c5f9..7ec0012 100644
--- a/README.md
+++ b/README.md
@@ -2,7 +2,7 @@
 
 ![](https://res.cloudinary.com/zeit-inc/image/upload/v1509936789/repositories/pkg/pkg-repo-banner-new.png)
 
-[![ci](https://github.com/vercel/pkg/actions/workflows/ci.yml/badge.svg)](https://github.com/vercel/pkg/actions/workflows/ci.yml)
+[![Build Status](https://github.com/vercel/pkg/actions/workflows/ci.yml/badge.svg)](https://github.com/vercel/pkg/actions/workflows/ci.yml)
 [![Coverage Status](https://coveralls.io/repos/github/vercel/pkg/badge.svg?branch=master)](https://coveralls.io/github/vercel/pkg?branch=master)
 [![Dependency Status](https://david-dm.org/vercel/pkg/status.svg)](https://david-dm.org/vercel/pkg)
 [![devDependency Status](https://david-dm.org/vercel/pkg/dev-status.svg)](https://david-dm.org/vercel/pkg?type=dev)
diff --git a/dictionary/.eslintrc.json b/dictionary/.eslintrc.json
index 39efc94..74e0826 100644
--- a/dictionary/.eslintrc.json
+++ b/dictionary/.eslintrc.json
@@ -2,4 +2,4 @@
   "parserOptions": {
     "sourceType": "script"
   }
-}
\ No newline at end of file
+}

@robertsLando
Copy link
Contributor Author

@leerob Could you squash and merge this? Needs admin priviledges, also in branch protection settings you should change the required checks and remove travis from required

@leerob leerob merged commit 31e8bf7 into master Mar 24, 2021
@leerob leerob deleted the workflows branch March 24, 2021 17:08
@jesec
Copy link
Contributor

jesec commented Mar 24, 2021

Ummm…. dependabot don’t know how to squash the PRs and started to spam the project. Even if all of them are promptly merged by maintainers, the commit history gets spammed.

I don’t think that’s good for the health of the project, as once in while, the automated PRs can drown more important PRs. Commit history full of automatic commits also makes it hard to write changelogs or locate more important commits.

I think we ought to figure out a different solution/configuration.

@leerob
Copy link
Member

leerob commented Mar 24, 2021

If there's an option to group PRs for Dependabot, I'm good with that.

@leerob leerob mentioned this pull request Mar 25, 2021
@robertsLando
Copy link
Contributor Author

We could use https://mergify.io/ to auto merge dev deps once checks pass. Thoughts?

@jesec
Copy link
Contributor

jesec commented Mar 25, 2021

We could use https://mergify.io/ to auto merge dev deps once checks pass. Thoughts?

Auto merging is not a good solution. Commit history still gets spammed. I prefer to configure dependabot to security-only or disable it altogether.

@robertsLando
Copy link
Contributor Author

Ok no problem for me

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

Successfully merging this pull request may close these issues.

3 participants