AEGEE / oms-core-first

[DEPRECATED] Core module of the online membership system of AEGEE.
Other
5 stars 5 forks source link

Added logging feature using bunyan #21

Closed stebenve86 closed 8 years ago

stebenve86 commented 8 years ago

A logging system (using bunyan library) has been added to the project. The logger is defined in the "logger.js" file; it can be called directly after requiring it ("var log = require('./logger.js')") and it is passed in the restify and ldap client instantiation (the two libraries have a built-in support for bunyan). A log folder has been added. The log file can be analyzed using the bunyan CLI command (node_modules/bunyan/bin/bunyan).

To be checked: 1) The modifyMembership function has been modified with a call to the searchLDAP function, the code seemed to be redundant 2) The bunyan package has been added in package.json. The versions of the existing packages have been updated to the latest ones, not sure if the previous versions were needed for other reasons

ingomueller-net commented 8 years ago

I only had a very short look and I will let @serviceman have the final word, but many changes look like great ideas. For the future I would wish to separate different changes into different commits (for example one for updating the versions, one for cleaning up whitespace, one for introducing the logger, etc). Nice work!

stebenve86 commented 8 years ago

In order to separate the changes, do you mean you prefer a new branch like this with 3 different commits or 3 different branches (and I think 3 pull requests)?

Regarding the merge of this branch, please wait until a test on the new modifyMembership function will be performed. I'll provide a positive or negative feedback (and I will commit another change to this branch if needed).

linuxbandit commented 8 years ago

@ingomueller-net other than what @stebenve86 asked about the PR&commits (he squashed the commits to have a cleaner history as it is good practice) I also have a question for you, are the versions in packages.json okay?

ingomueller-net commented 8 years ago

@stebenve86: Good question, I am not not entirely sure. I think that it is legitimate to make a single pull request with a new feature, and piggy-back whitespace fixes that you made "while you are at it". If the other changes become too involved, make a second PR. In other words: it depends ;)

@serviceman: I did not check whether things are still working. Some version changes are indeed incrementing the major version, which may break compatibility, so it should be a conscious decision. If I understand semantic versioning correctly, ~1 is equivalent to 1.*, which I think is fine since it should not make major version upgrades automatically. It is also (almost) equivalent to ^1.0.3, which I believe I have seen more often, but I am not sure... So: check whether you want to make a major upgrade; if so then the change is fine.

stebenve86 commented 8 years ago

Regarding the new modifyMembership function, it seems to be working.

With both old and new function it is possible to modify the "memberType" value of a aegeePersonMembership object (for the test it has been changed in the code in order to adapt to the test DB).

With the old function, where "res.send(200, results);" is outside the search block, the output is an empty object ("[]"). With the function of this new branch the output is the json object with the new modified data.

So, it seems to work in the correct way (or at least, with no evident regression).

ingomueller-net commented 8 years ago

Cool! I'll let @serviceman do the decision as the main developer.

@stebenve86, can you merge into dev, please? We haven't documented it yet, but we want to always merge into dev, test, and then merge into master.

linuxbandit commented 8 years ago

@stebenve86 we are using this branching model, so yes, please modify the request destination.

Notice also that meanwhile I am merging #22 in, so maybe you need to pull and integrate the new changes

stebenve86 commented 8 years ago

A new Pull Request will be opened in order to merge in the correct branch.