Add self-signed option to allow configure with private gitlab instances by jlengrand · Pull Request #32 · RamonGebben/mergify

jlengrand · 2018-04-27T09:17:01Z

Also add support for command options

Fixes #27

* Also add support for command options

coveralls · 2018-04-27T09:20:01Z

Pull Request Test Coverage Report for Build 56

6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.4%) to 96.429%

Totals
Change from base Build 46:	0.4%
Covered Lines:	54
Relevant Lines:	56

💛 - Coveralls

jlengrand · 2018-04-27T09:17:47Z

index.js

      .description(description)
      .action((...args) => fn(config, ...args));
+
+    if(options && options.length > 0){


I'm not happy about the way the JS is written here. @RamonGebben I could use your help to make it more beautiful :).

I ended up removing the logic since it is irrelevant at the moment

jlengrand · 2018-04-29T09:45:13Z

Some commands from @RamonGebben that I still have to process :

The option should be at software level, not command level (nothing should work without the flag)
Use index for command level options instead of weird command indexing.

I will leave the command level option logic so we can reuse it later.

* Remove logic for option specific command

jlengrand · 2018-04-29T10:08:25Z

Alright, all feedback processed. @RamonGebben , this is coming back to you :).

RamonGebben · 2018-04-27T11:27:54Z

index.js

+    if(options && options.length > 0){
+      options.forEach(
+        (option) => {
+          program.commands[program.commands.length - 1]


This index is a bit confusing to me. It looks like you're trying to get the current index.

Maybe grab the index of current iteration at line 62.

RamonGebben · 2018-04-27T11:30:45Z

lib/options/disableCertificateVerification/index.js

+const chalk = require('chalk');
+
+const disableCertificateVerification = async() => {
+    logger.log(chalk.red.bold('⚠️ Disabling certificate verification. This is unsafe and should only be used as last resort.'));


Let's extend the logger to have a .warn or .danger and have the chalk stuff in there.

Alright, I gave it a go. Let me see if you are happy about the implementation :).

RamonGebben · 2018-04-27T11:36:46Z

lib/options/disableCertificateVerification/index.js

+
+const disableCertificateVerification = async() => {
+    logger.log(chalk.red.bold('⚠️ Disabling certificate verification. This is unsafe and should only be used as last resort.'));
+    process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0";


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

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

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

* Avoid having to use chalk all the time

RamonGebben · 2018-05-01T04:32:58Z

Now the test coverage has dropped by 4.6% can you see if you can up that again as well?

RamonGebben · 2018-05-01T04:48:42Z

lib/utils/logger/index.js

 module.exports = {
-  logger: console
-};
+  logger: new Logger()


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.

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

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.

Nevermind, got it working!

RamonGebben · 2018-05-01T04:50:41Z

lib/commands/configure/index.js

 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.');


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

RamonGebben · 2018-05-01T04:57:11Z

lib/utils/logger/index.js

@@ -1,3 +1,19 @@
+const chalk = require('chalk');
+
+var Logger = function(){};


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.

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

jlengrand · 2018-05-01T06:41:13Z

Alright, all what's left now is to test.

jlengrand · 2018-05-01T07:29:23Z

Alright, I have something but I'm not happy about the tests. Can you please have a look at it?

RamonGebben · 2018-07-27T09:41:14Z

lib/utils/logger/index.spec.js

+
+    test('Testing normal log message', () => {
+
+        logger.log('test', 'plop');


Why not use mock functions from Jest?
https://jestjs.io/docs/en/mock-functions

Then you could getLogger with { log: jest.fn() }

const mockConsole = { log: jest.fn(), }; logger = getLogger(mockConsole); logger.log('Pizza') expect(mockConsole.log.mock.results[0].value).toBe('Pizza')

RamonGebben · 2018-07-27T09:42:50Z

lib/utils/logger/index.js

+
+const getLogger = function(transport){
+  return {
+    log : (...args) =>{


Formatting can be a little prettier:

return { log: (...args) => { }, }

RamonGebben · 2018-07-27T09:43:54Z

index.js

    await verify();
  }

+  options.forEach(({trigger, description, fn}) => {


Can we put whitespace around deconstruction arguments?

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

Julien Lengrand-Lambert added 2 commits April 27, 2018 10:40

Update mergify to use commands instead of options

b4ffada

Add self-signed option to allow configure with private gitlab instances

d48649c

* Also add support for command options

jlengrand changed the base branch from commands to master April 29, 2018 09:43

jlengrand commented Apr 29, 2018

View reviewed changes

jlengrand changed the title ~~Add self-signed option to allow configure with private gitlab instances~~ WIP: Add self-signed option to allow configure with private gitlab instances Apr 29, 2018

Julien Lengrand-Lambert added 2 commits April 29, 2018 11:52

Add logic for mergify level options

2b19b2a

* Remove logic for option specific command

Remove unnecessary options input in forEach

1ac4ac3

jlengrand requested a review from RamonGebben April 29, 2018 10:08

jlengrand changed the title ~~WIP: Add self-signed option to allow configure with private gitlab instances~~ Add self-signed option to allow configure with private gitlab instances Apr 29, 2018

RamonGebben reviewed Apr 30, 2018

View reviewed changes

Create more advanced logger

aef29ed

* Avoid having to use chalk all the time

RamonGebben requested changes May 1, 2018

View reviewed changes

RamonGebben reviewed May 1, 2018

View reviewed changes

Add emoticons to logger

e45afa6

Julien Lengrand-Lambert added 4 commits May 1, 2018 08:41

Use object literal instead of constructor

28a1440

Merge master

64da3d0

Remove duplication

44f309e

Start adding tests

89fdc01

RamonGebben reviewed Jul 27, 2018

View reviewed changes

RamonGebben assigned jlengrand Aug 14, 2019

		@@ -1,3 +1,19 @@
		const chalk = require('chalk');

		var Logger = function(){};


		test('Testing normal log message', () => {

		logger.log('test', 'plop');

Conversation

jlengrand commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 56

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlengrand commented Apr 29, 2018

Uh oh!

jlengrand commented Apr 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RamonGebben commented May 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlengrand commented May 1, 2018

Uh oh!

jlengrand commented May 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jlengrand commented Apr 27, 2018 •

edited

Loading

coveralls commented Apr 27, 2018 •

edited

Loading