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

Testing ignore script - RSC-980 #704

Draft
wants to merge 35 commits into
base: main
Choose a base branch
from
Draft

Conversation

entroform
Copy link
Contributor

@entroform entroform commented Apr 19, 2022

@@ -15,3 +15,5 @@ RUN apt-get update && \
RUN useradd -u 1002 -g 100 jenkins
RUN mkdir -p /home/jenkins/.npm
Copy link

Choose a reason for hiding this comment

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

DL3059: Multiple consecutive RUN instructions. Consider consolidation.

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -15,3 +15,5 @@ RUN apt-get update && \
RUN useradd -u 1002 -g 100 jenkins
RUN mkdir -p /home/jenkins/.npm
Copy link

Choose a reason for hiding this comment

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

DL3059: Multiple consecutive RUN instructions. Consider consolidation.

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -15,3 +15,5 @@ RUN apt-get update && \
RUN useradd -u 1002 -g 100 jenkins
RUN mkdir -p /home/jenkins/.npm
Copy link

Choose a reason for hiding this comment

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

DL3059: Multiple consecutive RUN instructions. Consider consolidation.

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -15,3 +15,5 @@ RUN apt-get update && \
RUN useradd -u 1002 -g 100 jenkins
RUN mkdir -p /home/jenkins/.npm
Copy link

Choose a reason for hiding this comment

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

DL3059: Multiple consecutive RUN instructions. Consider consolidation.

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -15,3 +15,5 @@ RUN apt-get update && \
RUN useradd -u 1002 -g 100 jenkins
RUN mkdir -p /home/jenkins/.npm
Copy link

Choose a reason for hiding this comment

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

DL3059: Multiple consecutive RUN instructions. Consider consolidation.

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -15,3 +15,5 @@ RUN apt-get update && \
RUN useradd -u 1002 -g 100 jenkins
RUN mkdir -p /home/jenkins/.npm
Copy link

Choose a reason for hiding this comment

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

DL3059: Multiple consecutive RUN instructions. Consider consolidation.

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -15,3 +15,5 @@ RUN apt-get update && \
RUN useradd -u 1002 -g 100 jenkins
RUN mkdir -p /home/jenkins/.npm
Copy link

Choose a reason for hiding this comment

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

DL3059: Multiple consecutive RUN instructions. Consider consolidation.

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -15,3 +15,5 @@ RUN apt-get update && \
RUN useradd -u 1002 -g 100 jenkins
RUN mkdir -p /home/jenkins/.npm
Copy link

Choose a reason for hiding this comment

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

DL3059: Multiple consecutive RUN instructions. Consider consolidation.

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -15,3 +15,5 @@ RUN apt-get update && \
RUN useradd -u 1002 -g 100 jenkins
RUN mkdir -p /home/jenkins/.npm
Copy link

Choose a reason for hiding this comment

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

DL3059: Multiple consecutive RUN instructions. Consider consolidation.

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -15,3 +15,5 @@ RUN apt-get update && \
RUN useradd -u 1002 -g 100 jenkins
RUN mkdir -p /home/jenkins/.npm
Copy link

Choose a reason for hiding this comment

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

DL3059: Multiple consecutive RUN instructions. Consider consolidation.

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -15,3 +15,5 @@ RUN apt-get update && \
RUN useradd -u 1002 -g 100 jenkins
RUN mkdir -p /home/jenkins/.npm
Copy link

Choose a reason for hiding this comment

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

DL3059: Multiple consecutive RUN instructions. Consider consolidation.

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -15,3 +15,5 @@ RUN apt-get update && \
RUN useradd -u 1002 -g 100 jenkins
RUN mkdir -p /home/jenkins/.npm
Copy link

Choose a reason for hiding this comment

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

DL3059: Multiple consecutive RUN instructions. Consider consolidation.

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -15,3 +15,5 @@ RUN apt-get update && \
RUN useradd -u 1002 -g 100 jenkins
RUN mkdir -p /home/jenkins/.npm
Copy link

Choose a reason for hiding this comment

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

DL3059: Multiple consecutive RUN instructions. Consider consolidation.

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

Comment on lines +153 to +154
<!-- Node 10.x or 12.x. Node 14.x is known not to work currently. -->
Node 12.x
Copy link
Contributor

Choose a reason for hiding this comment

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

This is out of date even before this PR. We're on Node 16 now and also support 14

!.yarn/plugins
!.yarn/releases
!.yarn/sdks
!.yarn/versions
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a top-level .gitignore which woul be the right place for these entries that are in both lib and gallery

"axios": "^0.21.1",
"classnames": "^2.2.6",
"compare-versions": "^4.1.3",
"core-js": "^3.8.3",
"es6-object-assign": "^1.1.0",
"es6-set": "^0.1.5",
"link": "^1.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's this?

Oh I meant to remove that!

@rpokorny
Copy link
Contributor

Could you explain what it is about yarn 2+ that enables the installation of puppeteer without running its install scripts?

@rpokorny
Copy link
Contributor

I suspect an alternative to switching our yarn version would be to use puppeteer's environment variables to disable the download and provide it the path to the chromium executable already present on the system. See https://dev.to/maxprogramming/how-to-skip-chromium-download-in-puppeteer-2c24

@entroform
Copy link
Contributor Author

Could you explain what it is about yarn 2+ that enables the installation of puppeteer without running its install scripts?

So in yarnrc.yml
I set it so it disables script by default:

enableScripts: false

Then in package.json, I added this configuration to allow scripts on a specific module:

 "dependenciesMeta": {
    "puppeteer": {
      "built": true
    }
  },

https://yarnpkg.com/configuration/manifest#dependenciesMeta.built

@entroform
Copy link
Contributor Author

entroform commented May 2, 2022

I suspect an alternative to switching our yarn version would be to use puppeteer's environment variables to disable the download and provide it the path to the chromium executable already present on the system. See https://dev.to/maxprogramming/how-to-skip-chromium-download-in-puppeteer-2c24

Thanks! I'll see if I can get this to work on another branch!

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.

2 participants