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.

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

feat: ensure every log file has at least one record #1414

Merged
merged 3 commits into from
Sep 7, 2019

Conversation

mlucool
Copy link
Contributor

@mlucool mlucool commented Jul 29, 2019

Type: feature

The following has been addressed in the PR:
Description:
Sometimes logs are empty if no one tries to access verdaccio. This is especially with logrotate where no one may access it over a period of time. This commit ensures every log has at least one message in it (assuming warn or above level of logging).

@juanpicado
Copy link
Member

I will check it later, but, I am rethinking about file rotation in the next major release next year and remove such support. Thoughts ?

@mlucool
Copy link
Contributor Author

mlucool commented Jul 30, 2019

Why would you want to remove that? IMO, it's a great and useful feature!

This issue comes up without file rotate also, hence the Verdaccio started log statement.

@juanpicado
Copy link
Member

There is a issue, the message get printed twice on startup with the default config.

@mlucool
Copy link
Contributor Author

mlucool commented Aug 2, 2019

There is a issue, the message get printed twice on startup with the default config.

Not sure if that's a bug. The issue is that in the default logging, we call setup twice and both happen to output to the same place (console). First in cli.ts then in index.ts. In theory these are different logs. I can add something to make cli.js not add this output if you think that's cleaner.

@juanpicado
Copy link
Member

Not sure if that's a bug. The issue is that in the default logging, we call setup twice and both happen to output to the same place (console). First in cli.ts then in index.ts. In theory these are different logs. I can add something to make cli.js not add this output if you think that's cleaner.

It is a design decision by my predecesor. Since we have this in in progress #1334 I wouldn't no change that. I'll add it to the check list, but better don't do it in this PR. Logger will change soon or later, I agree there are pitfalls like that one you described.

@mlucool
Copy link
Contributor Author

mlucool commented Aug 10, 2019

So what should we do to move this forward?

@juanpicado
Copy link
Member

juanpicado commented Aug 10, 2019

I was waiting your comments, but we need to remove logger.warn('Verdaccio started'); (one of them), since it pops up twice and does not look good in the terminal.

@mlucool
Copy link
Contributor Author

mlucool commented Aug 12, 2019

The above comment has been fixed

@mlucool
Copy link
Contributor Author

mlucool commented Aug 26, 2019

Is there anything else that should be fixed?

@juanpicado
Copy link
Member

hi @mlucool I could not test it yet, completely forgot it. perhaps cc: @verdaccio/collaborators can help checking this to ship it asap, or maybe me a long the week.

@juanpicado juanpicado added this to the 4.3.0 milestone Aug 31, 2019
Copy link
Member

@juanpicado juanpicado left a comment

Choose a reason for hiding this comment

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

👍 now works fine 👍

 warn --- config file  - /Users/xxxx/.config/verdaccio/config.yaml
 warn --- Verdaccio started
 warn --- Plugin successfully loaded: verdaccio-htpasswd
 warn --- Plugin successfully loaded: verdaccio-audit
 warn --- http address - http://localhost:4873/ - verdaccio/4.2.2

@juanpicado juanpicado merged commit 962d5d5 into verdaccio:master Sep 7, 2019
@lock
Copy link

lock bot commented Sep 17, 2019

🤖This thread has been automatically locked 🔒 since there has not been any recent activity after it was closed.
We lock tickets after 90 days with the idea to encourage you to open a ticket with new fresh data and to provide you better feedback 🤝and better visibility 👀.
If you consider, you can attach this ticket 📨 to the new one as a reference for better context.
Thanks for being a part of the Verdaccio community! 💘

@lock lock bot locked as resolved and limited conversation to collaborators Sep 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants