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

Fix directory permissions for assessment imports #1906

Merged
merged 13 commits into from
May 21, 2023

Conversation

20wildmanj
Copy link
Contributor

@20wildmanj 20wildmanj commented May 19, 2023

Description

This PR addresses failing unit tests in assessments_controller_spec.rb for importAsmtFromTar by correctly setting directory permissions.

Motivation and Context

Tests were failing due to

  1. Archive.in_dir? check being strict, meaning that we skipped over the assessment directory itself
  2. When entry is a file, entry.header.mode is passed to mkdir_p, which is in general incorrect. This is because modes might be specified with the file type, e.g. 100644 indicates a file with permissions 0644. See https://unix.stackexchange.com/a/450488 for more details.

Screenshot 2023-05-21 at 03 20 14

This resulted in directories with bad permissions if we process a file and

  1. Parent directory was not created yet, either because we skipped over it, or possibly the directory entry comes later, and no other files in the directory were processed yet
  2. The file has a type specific mode

To remedy this, the PR

  1. defaults to mode 0755 for the creation of parent directories when processing a file
  2. chmod when processing directories, to ensure that have the correct permissions if they were created implicitly when processing a file
  3. change the in_dir? check to allow for equality

How Has This Been Tested?

Run bundle exec rails spec

See that importing assessments still work manually:

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
  • My change requires a change to the documentation, which is located at Autolab Docs
  • I have updated the documentation accordingly, included in this PR

@damianhxy damianhxy force-pushed the joeywildman-fix-asmt-import-test branch 2 times, most recently from 5d5e057 to 4261bc1 Compare May 20, 2023 17:39
@damianhxy damianhxy force-pushed the joeywildman-fix-asmt-import-test branch from 4261bc1 to d72dd6b Compare May 20, 2023 17:44
@damianhxy damianhxy changed the title Remove macOS specific files from asmt import tar Fix directory permissions for assessment imports May 20, 2023
Copy link
Member

@damianhxy damianhxy left a comment

Choose a reason for hiding this comment

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

LGTM by @20wildmanj

@20wildmanj 20wildmanj added this pull request to the merge queue May 21, 2023
Merged via the queue into master with commit 41402d1 May 21, 2023
@20wildmanj 20wildmanj deleted the joeywildman-fix-asmt-import-test branch May 21, 2023 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants