-
Notifications
You must be signed in to change notification settings - Fork 0
MI-206: Have CI_DEBUG affect package manager install command(s) #13
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
Conversation
This change allows us to see more detail when the package manager fails for whatever reason. For most package managers, the specific error message when not using `--verbose` (for example) only include a fairly generic error, even if the error is unrecoverable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look great, @toddhainsworth
I'm so happy that there is someone else who cares about this repo.
lib/packageManagers.ts
Outdated
return 'yarn install --frozen-lockfile'; | ||
export function getInstallCommand( | ||
packageManager: PackageManager, | ||
debug: boolean = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just import the global debug environment variables instead of passing it in as a parameter like this. Check out how it is setup in injectCfnRole.ts
file:
import { env } from './env';
if (env.debug) {
console.log(
logSymbols.info,
chalk.whiteBright(JSON.stringify(serverless, null, 2))
);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good pickup! Thanks, I'll update.
const installCommands: Record<PackageManager, string> = { | ||
npm: 'npm ci', | ||
pnpm: 'pnpm install --frozen-lockfile', | ||
yarn: 'yarn install --frozen-lockfile', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update this. yarn
no longer use --frozen-lockfile
. the correct flag is now --immutable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another great pickup! Do we need to decide based on the Yarn version or are we happy to only support versions >= the version that introduced --immutable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's there in Yarn Berry since version 2.0.0. I think it's safe for us to assume that no one ever run this pipeline in Yarn Classic. If they ever do it, probably they made another mistake and need to upgrade their package manager to Yarn Berry 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent - pushed a change to use --immutable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, Use --immutable for Yarn and debug from environment didn't actually use --immutable
🤣 .
Thanks guys, ready for another review. |
This change allows us to see more detail when the package manager fails for whatever reason. For most package managers, the specific error message when not using
--verbose
(for example) only include a fairly generic error, even if the error is unrecoverable.