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

Jump to bottom

Stronger condition to check NFS #17695

Merged
merged 1 commit into from Jun 29, 2021
Merged

Stronger condition to check NFS #17695

merged 1 commit into from Jun 29, 2021

Conversation

Seb35
Copy link
Contributor

@Seb35 Seb35 commented Jun 18, 2021

Description:

Issue: #12217 - in the system check page, filesystems like btrfs subvolumes are wrongly recognised as NFS.

NFS filesystems are identified as 'nfs' or 'nfs4' by df -T, so the added condition tests the existence of ' nfs'.

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@Seb35
Copy link
Contributor Author

Seb35 commented Jun 18, 2021 •
edited

I tested on my Matomo installation where the directory tmp is a btrfs subvolume (and tmp/sessions is inside): the check now passes.

@sgiehl sgiehl added the Needs Review PRs that need a code review label Jun 18, 2021
@github-actions
Copy link
Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jun 26, 2021
diosmosis
diosmosis reviewed Jun 27, 2021
View reviewed changes
core/Filesystem.php Outdated Show resolved Hide resolved
diosmosis
diosmosis requested changes Jun 27, 2021
View reviewed changes
Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

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

Hi @Seb35, thanks for going through the trouble of creating this PR! I've requested a single change, otherwise it looks good to merge!

@Seb35
Stronger condition to check NFS
bc678c4 
Exclude unrecognized filesystems like btrfs subvolumes.
NFS filesystems are identified as 'nfs' or 'nfs4'.

Issue: matomo-org#12217
@Seb35
Copy link
Contributor Author

Seb35 commented Jun 28, 2021

I did the requested change, it is now more robust.

@github-actions github-actions bot removed the Stale The label used by the Close Stale Issues action label Jun 29, 2021
@diosmosis diosmosis added this to the 4.4.0 milestone Jun 29, 2021
@diosmosis
Copy link
Member

@Seb35 I tested locally and it works well, thanks again for creating this PR and contributing some code! We certainly appreciate the effort.

@diosmosis diosmosis merged commit 0c61a75 into matomo-org:4.x-dev Jun 29, 2021
@diosmosis diosmosis linked an issue Jun 29, 2021 that may be closed by this pull request
Bad filesystem detection (NFS) #12217
Closed
@diosmosis diosmosis mentioned this pull request Jun 29, 2021
Closed
@mattab mattab added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diosmosis diosmosis diosmosis requested changes

Assignees
No one assigned
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.4.0
Development

Successfully merging this pull request may close these issues.

Bad filesystem detection (NFS)
4 participants
@Seb35 @diosmosis @mattab @sgiehl