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.

Sign up for GitHub

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

Jump to bottom

Fixing Configuration-related Bugs #130

Merged
merged 38 commits into from
Oct 23, 2019
Merged

Fixing Configuration-related Bugs #130

merged 38 commits into from
Oct 23, 2019

Conversation

smusali
Copy link
Contributor

@smusali smusali commented Oct 7, 2019

this covers the followings:

  • being able to override LOGDNA_LOGHOST and/or LOGDNA_LOGPORT thru config file
  • being able to use different config file
  • ignoring to do file reading operation if it is on Windows
  • simplifying getting OS Distro information
  • rearranging utility functions
  • moving custom logger into ./lib/utils.js
  • more informative debug messages

@smusali smusali added the bugfix label Oct 7, 2019
@smusali smusali requested review from mikehu and dchai76 October 7, 2019 15:16
@smusali smusali self-assigned this Oct 7, 2019
@smusali
Copy link
Contributor Author

smusali commented Oct 7, 2019

@mikehu @dchai76 can you review this Pull Request if possible? Thanks in advance!

P.S. Travis fails due to cached build.

dchai76
dchai76 suggested changes Oct 8, 2019
View reviewed changes
test/lib/getDistro.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
lib/connection-manager.js Outdated Show resolved Hide resolved
@smusali smusali requested a review from dchai76 October 8, 2019 19:17
dchai76
dchai76 suggested changes Oct 8, 2019
View reviewed changes
Copy link
Contributor

@dchai76 dchai76 left a comment

Choose a reason for hiding this comment

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

Do me a favor and ask @mikehu for his take on the config thing. My gut feels like it's wrong to adjust it that way.

Gruntfile.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@smusali smusali requested a review from dchai76 October 9, 2019 02:49
@smusali
Copy link
Contributor Author

smusali commented Oct 9, 2019

@mikehu can you give your final feedback after reviewing if possible? Thanks in advance!

@smusali
Copy link
Contributor Author

smusali commented Oct 9, 2019

@dchai76 @mikehu I'd say I could let them be separate local variable without getting added to config

dchai76
dchai76 reviewed Oct 9, 2019
View reviewed changes
Copy link
Contributor

@dchai76 dchai76 left a comment

Choose a reason for hiding this comment

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

This looks a lot better to me, but let me defer to @mikehu to approve.

@smusali smusali requested a review from dchai76 October 9, 2019 22:26
mikehu
mikehu suggested changes Oct 11, 2019
View reviewed changes
index.js Outdated Show resolved Hide resolved
lib/get-distro.js Outdated Show resolved Hide resolved
lib/get-distro.js Outdated Show resolved Hide resolved
mikehu
mikehu suggested changes Oct 11, 2019
View reviewed changes
lib/linebuffer.js Outdated Show resolved Hide resolved
@smusali
Copy link
Contributor Author

smusali commented Oct 17, 2019

@mikehu, changes are done! Can you take a final look over the changes? Thanks in advance! :)

@smusali smusali requested a review from mikehu October 17, 2019 03:05
@smusali
Copy link
Contributor Author

smusali commented Oct 21, 2019

@mikehu, can you do final round of review? Thanks in advance!

@dchai76 dchai76 requested a review from weizou19 October 23, 2019 00:30
@smusali smusali merged commit 2ce941d into master Oct 23, 2019
@smusali smusali deleted the fixConfig branch October 23, 2019 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikehu mikehu mikehu approved these changes

@dchai76 dchai76 Awaiting requested review from dchai76

@weizou19 weizou19 Awaiting requested review from weizou19

Assignees

@smusali smusali

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@smusali @mikehu @dchai76