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

Reduce legacy event log level to debug #18966

Closed
wants to merge 1 commit into from
Closed

Conversation

MichaIng
Copy link
Member

@MichaIng MichaIng commented Jan 18, 2020

This is to reduce log flooding on info log level, which is currently expected due to deprecated event use in many apps and core: #18331
This information is helpful for developers only, hence should be sufficient as debug log. Especially due to the extremely high frequency this log can happen, it currently practically forces admins to disable info logs, which conflicts with other needs.

Fixes #18331, Fixes #19097

Signed-off-by: MichaIng micha@dietpi.com

@kesselb
Copy link
Contributor

kesselb commented Jan 18, 2020

Thanks 👍

@ChristophWurst
Copy link
Member

How about debug for 18, where we can't change much at this stage and a warning for 19+ where we hopefully can finish the migration and thus it should only pop up in edge cases?

I just want to avoid having to keep compat for ages. The sooner we can get this done the better ✌️

@kesselb kesselb changed the base branch from master to stable18 January 18, 2020 13:22
@kesselb kesselb changed the base branch from stable18 to master January 18, 2020 13:22
@kesselb
Copy link
Contributor

kesselb commented Jan 18, 2020

Fine by me. @MichaIng could you rebase the branch to stable18 and change the base?

@MichaIng
Copy link
Member Author

@ChristophWurst
I just want to avoid bothering local admins with this, since this is a developer topic only. If the new event type should be used quickly everywhere, wouldn't it then make sense to simply remove compat code and have apps, using the old type, simply broken? Of course core needs to be completely migrated first and it must be communicated via dev changelog and the API/dev changes issue I saw here. This early stage of NC19 should give enough time to all app devs to migrate?

@ChristophWurst
Copy link
Member

If the new event type should be used quickly everywhere, wouldn't it then make sense to simply remove compat code and have apps, using the old type, simply broken? Of course core needs to be completely migrated first and it must be communicated via dev changelog and the API/dev changes issue I saw here. This early stage of NC19 should give enough time to all app devs to migrate?

Yes. Unfortunately the breakage of the Symfony event was detected a bit late in the 18 dev cycle, that is why we have the old and new version.

@MichaIng MichaIng changed the base branch from master to stable18 January 18, 2020 16:07
@MichaIng MichaIng changed the base branch from stable18 to master January 18, 2020 16:09
@MichaIng
Copy link
Member Author

MichaIng commented Jan 18, 2020

Hmm, this cannot be done from GitHub web UI, can it? https://gist.github.com/txels/178c78a8ffb8b7e8b79b
Currently cloning and trying from desktop client.

@kesselb
Rebase done, puuh took me a while to add --force option out of despair to finally resolve this 😅:

 ! [rejected]              legacy_event_log -> legacy_event_log (non-fast-forward)
error: failed to push some refs to 'https://github.com/nextcloud/server.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

This is to reduce log flooding on info log level, which is currently expected tue to deprecated event use in many apps and core: #18331
This information is helpful for developers only, hence should be sufficient as debug log. Especially due to the extremely high frequency this log can happen, this currently practically forces admins to disable info logs, which conflicts with other needs.

Signed-off-by: MichaIng <micha@dietpi.com>
@MichaIng MichaIng changed the base branch from master to stable18 January 18, 2020 17:12
@MichaIng
Copy link
Member Author

Second reviewer can approve this now? Would be great to have it in NC18 release. For NC19 it is indeed best to keep the logs more prominent or even raise them to warnings, to constantly remember all developers about the event type migration 😉.

@juliusknorr juliusknorr added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews backport-request labels Jan 22, 2020
@nextcloud nextcloud deleted a comment from kesselb Jan 22, 2020
@juliusknorr
Copy link
Member

Deleted the backport comment since base branch is changed to stable18

@kesselb
Copy link
Contributor

kesselb commented Jan 22, 2020

So we will never find out if the backportbot is smart enough to figure out that source and target are the same 🤣 🙄

@juliusknorr
Copy link
Member

But we will find out if it checks if the comment is still present or does weird things now 😉

@MichaIng
Copy link
Member Author

No worries, since I know now how to change a branches base branch, I can create some other PR with the aim to have the backportbot pushing its self-destruction button 😄.

@rullzer
Copy link
Member

rullzer commented Jan 23, 2020

Ah I think drone chokes on chaging the base branch

@MichaIng
Copy link
Member Author

@rullzer
Seems so. I mean the commit is very trivial, but I can open a fresh PR against stable18 to satisfy CI drone, should I?

@rullzer
Copy link
Member

rullzer commented Jan 24, 2020

@rullzer
Seems so. I mean the commit is very trivial, but I can open a fresh PR against stable18 to satisfy CI drone, should I?

yesplease do.
Just closing this and opening a new one on the branch should do the trick.

@rullzer
Copy link
Member

rullzer commented Jan 24, 2020

So we will never find out if the backportbot is smart enough to figure out that source and target are the same rofl roll_eyes

It is. But mainly because it is a stupid lazy bot so it will only check once the PR is merged 📦

But we will find out if it checks if the comment is still present or does weird things now wink

Also here. Because it is a stupid lazy stateless bot :P It will only see commits actually present :P

@MichaIng
Copy link
Member Author

MichaIng commented Jan 24, 2020

mainly because it is a stupid lazy
...
Also here. Because it is a stupid lazy

That serves me some arguments against my boss, wife and mama 👍.

Okay lets get those lazy guys some job to do. New PR: #19118

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish feature: logging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants