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

Do not empty config.php file if reading failed for any reason #33921

Merged
merged 1 commit into from
Sep 12, 2022
Merged

Do not empty config.php file if reading failed for any reason #33921

merged 1 commit into from
Sep 12, 2022

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Sep 6, 2022 •
edited by szaimen
Loading

Fixes #18620

Signed-off-by: Côme Chilliet come.chilliet@nextcloud.com

@come-nc come-nc added the 2. developing Work in progress label Sep 6, 2022
@come-nc come-nc added this to the Nextcloud 25 milestone Sep 6, 2022
@come-nc come-nc self-assigned this Sep 6, 2022
@come-nc
Do not empty config.php file if reading failed for any reason
c2dafb0 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@blizzz blizzz mentioned this pull request Sep 6, 2022
Merged
@szaimen
Copy link
Contributor

szaimen commented Sep 6, 2022

Seens like the tests work now? :)

@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 8, 2022
@come-nc come-nc requested review from szaimen, a team, ArtificialOwl, icewind1991 and skjnldsv and removed request for a team September 8, 2022 07:23
@come-nc
Copy link
Contributor Author

come-nc commented Sep 8, 2022

Seens like the tests work now? :)

Yes 😄

@szaimen
Copy link
Contributor

szaimen commented Sep 8, 2022

/backport to stable24

@szaimen
Copy link
Contributor

szaimen commented Sep 8, 2022

/backport to stable23

@szaimen
Copy link
Contributor

szaimen commented Sep 8, 2022

@come-nc can you please link the issues that get fixed by this? Thanks! :)

@come-nc
Copy link
Contributor Author

come-nc commented Sep 8, 2022

Fixes #18620

solracsf
solracsf approved these changes Sep 8, 2022
View reviewed changes
Copy link
Member

@solracsf solracsf left a comment

Choose a reason for hiding this comment

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

🥳

@solracsf
Copy link
Member

solracsf commented Sep 8, 2022 •
edited
Loading

@come-nc should also fix #25175 or it doesn't cover these cases?

@come-nc
Copy link
Contributor Author

come-nc commented Sep 8, 2022

@come-nc should also fix #25175 or it doesn't cover these cases?

No it does not fix this one. It fixes cases where reading failed, not the ones where writing fails.
Maybe replacing ftruncate by opening the file in write mode would solve those? Not sure.

@andrey-utkin
Copy link

Maybe replacing ftruncate by opening the file in write mode would solve those? Not sure.

No. Create new + rename would solve this, as suggested 1, 2

I'm just following the issue, would get my hands on it but you seem to be almost done :)

@blizzz blizzz mentioned this pull request Sep 9, 2022
Merged
@andrey-utkin andrey-utkin mentioned this pull request Sep 9, 2022
Merged
@andrey-utkin
Copy link

andrey-utkin commented Sep 9, 2022 •
edited
Loading

I've created that PR to demonstrate the atomic update approach. To be continued.

@come-nc come-nc merged commit 1f7e769 into master Sep 12, 2022
@come-nc come-nc deleted the fix/fix-config-file-emptied branch September 12, 2022 07:43
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Sep 12, 2022
Merged
@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

@szaimen
Copy link
Contributor

szaimen commented Sep 12, 2022

/backport to stable23

@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

@szaimen szaimen mentioned this pull request Sep 12, 2022
Merged
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
Merged
@joshtrichards joshtrichards mentioned this pull request Jul 11, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@solracsf solracsf solracsf approved these changes

@szaimen szaimen szaimen approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl

@icewind1991 icewind1991 Awaiting requested review from icewind1991

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees

@come-nc come-nc

Labels
3. to review Waiting for reviews bug high
Projects
None yet
Milestone
Nextcloud 25
Development

Successfully merging this pull request may close these issues.

complete config reset with filled disk
4 participants
@come-nc @szaimen @solracsf @andrey-utkin