Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const { configure } = require('./lib/commands/configure');
const { verify } = require('./lib/commands/verify');
const { getAllAssigned } = require('./lib/commands/getAllAssigned');
const { getAllSubmitted } = require('./lib/commands/getAllSubmitted');

const { disableCertificateVerification } = require('./lib/options/disableCertificateVerification');
const { readConfig } = require('./lib/utils/readConfig');

program
Expand All @@ -17,22 +17,30 @@ program

const options = [
{
trigger: '-a --assigned',
trigger: '-s --self-signed',
description: 'disables the verification of certificates for commands',
fn: disableCertificateVerification
}
];

const commands = [
{
trigger: 'assigned',
description: 'Get all open merge request assigned to you',
fn: getAllAssigned
},
{
trigger: '-s --submitted',
trigger: 'submitted',
description: 'Get all open merge request submitted to you',
fn: getAllSubmitted
},
{
trigger: '-c --configure',
trigger: 'configure',
description: 'Setup or update required config',
fn: configure
},
{
trigger: '-v --verify',
trigger: 'verify',
description: 'Verify your config is correct',
fn: verify
}
Expand All @@ -52,10 +60,18 @@ const run = async() => {
await verify();
}

options.forEach(({ trigger, description, fn }) => {
options.forEach(({trigger, description, fn}) => {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can we put whitespace around deconstruction arguments?

({ a, b }) => //...

program.option(trigger, description, (...args) => fn(config, ...args));
});

commands.forEach(({ trigger, description, fn }) => {
program
.command(trigger)
.description(description)
.action((...args) => fn(config, ...args));

});

return program;
};

Expand Down
2 changes: 1 addition & 1 deletion lib/commands/configure/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const chalk = require('chalk');

const configure = async() => {
if(await checkConfigExists()){
logger.log(chalk.red.bold('⚠️ Mergify is already configured. Configuring again will override the existing file.'));
logger.warn('⚠️ Mergify is already configured. Configuring again will override the existing file.');
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Since all warnings have a ⚠️ in front maybe also put those in the logger.
Same goes for the Uh-oh cat 🙀.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

}

try {
Expand Down
2 changes: 1 addition & 1 deletion lib/commands/verify/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const verify = async({ userId }) => {
} catch (error) {
spinner.stop();
logger.log('\n🙀 Oh no, could not complete verify. Please review your config');
logger.log(error);
logger.err(error);
process.exit(1);
}
};
Expand Down
11 changes: 11 additions & 0 deletions lib/options/disableCertificateVerification/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
const {logger} = require('../../utils/logger');

const disableCertificateVerification = async() => {
logger.warn('⚠️ Disabling certificate verification. This is unsafe and should only be used as last resort.');
process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0";
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Doesn't this need to be set every time the user calls mergify?
Every command you give to mergify is like calling a script so that would be a new process and the function is only invoked from the configure command.
So I think you want to write into the config that the user has set this. And check it before doFetch

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

On second glance, after reviewing the command refactoring. This flag will need to available on each command.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I do kinda agree with the comment. Thinking about #11 for example, some of the options should definitely be part of the config.

I think that this first version of the implementation is probably good enough for now though. Let's see what happens

return;
};

module.exports = {
disableCertificateVerification
};
20 changes: 18 additions & 2 deletions lib/utils/logger/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,19 @@
const chalk = require('chalk');

var Logger = function(){};
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Let's turn this into a getLogger function and pass a transport as the first argument.
Which returns the object literal which I was talking about down below.

That way you can provide a mocked version of console in your tests. And naturally this would have more options for the future as well.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No idea what you're talking about. This is all Chinese at the moment


Logger.prototype.log = function(...args) {
console.log(...args);
}

Logger.prototype.warn = function(...args) {
console.log(chalk.yellow.bold(...args));
}

Logger.prototype.err = function(...args) {
console.log(chalk.red.bold(...args));
}

module.exports = {
logger: console
};
logger: new Logger()
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure if we need a constructor for this. I think we should just use an object literal since we don't need to access anything in the this context.

So maybe like
const logger = { warn: (...args) => console.log(...args) }

That way we don't need to new it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hum, that's how I had it first but I didn't get it working.
Let me try again

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That's what I get if I try to use an object literal

/Users/jlengrand/IdeaProjects/mergify/lib/utils/logger/index.js:4
  log = (...args) =>{
  ^^^^^^^^^^^^^^^^^^^

SyntaxError: Invalid shorthand property initializer

Seems like variable arguments is not supported in the same way in there.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Nevermind, got it working!

}