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

Ensure hidden files are migrated to asset manager #3658

Merged
merged 7 commits into from Jan 4, 2018

Conversation

Projects
None yet
2 participants
@chrislo
Copy link
Contributor

commented Jan 4, 2018

C.f alphagov/asset-manager#368

We noticed that some assets in Whitehall have file names which include a leading dot[1] These files are treated as "hidden" by Ruby's Dir.glob by default and were therefore not being included in the asset_manager:migrate_assets rake task.

In this PR I first refactor the tests for the migration code to allow me to more easily fix an issue with hidden NFS lock files that occurs when running the tests on the development VM on an OS X host. I then fix that issue. In the final commit I fix the issue described above.

We have already run the rake task against a number of directories. In a subsequent PR I'll introduce another task that ensures that any hidden files we've missed are also migrated.

[1] e.g. /data/uploads/whitehall/clean/system/uploads/consultation_response_form_data/file/257/.doc_Response_form.docx.

chrislo added some commits Jan 3, 2018

Extract some methods from test setup
This simplifies the setup blocks for the tests and allows us to more
easily reuse the values in subsequent commits.
Teardown tests correctly
It is a good idea to ensure that the directories created in the setup
are removed in the teardown to avoid any leaking of state between tests.
Remove unused instance variables
These variables are not used in the subsequent test blocks so can be
removed.
Remove unnecessary instance variable
We already have a method for the path to a file so this variable is unnecessary.
Ensure opened files are closed
It is best practice to ensure that an opened file is closed using the
block form of `File#open` to free up the file descriptor.

On the development VM under an OS X host I noticed that before this
commit files starting `.nfs` would be created on the filesystem when
running these tests. In a subsequent commit I want to change the
implementation to glob for all files starting with `.`. This change
makes the subsequent one easier to test.
@floehopper
Copy link
Contributor

left a comment

Minor: The formatting of the PR description and the commit note for the "Include hidden files when migrating to Asset Manager" commit makes them hard to read. I think it might be worth extracting the long hidden file path into a Markdown reference/footnote.

Minor: Using the "Fixes: " directive will close that issue when this PR is merged. Is that what you want to happen? I often keep the associated issue open until I've checked that stuff has been deployed/applied correctly in all environments and so I tend to avoid using those kind of directives.

I'm happy to leave it to you to decide how/whether to address my minor comments. Marking as approved.

hidden_path = File.join(organisation_logo_dir, '.hidden.jpg')
FileUtils.cp(dummy_asset_path, hidden_path)

assert_equal 2, @subject.file_paths.size

This comment has been minimized.

Copy link
@floehopper

floehopper Jan 4, 2018

Contributor

Minor: This assertion seems quite vague and not easy to understand at first glance. Would it not be better to check for the inclusion of the hidden file's path? I see that some of the existing tests take a similar approach, so I guess it would be worth changing them as well...

This comment has been minimized.

Copy link
@chrislo

chrislo Jan 4, 2018

Author Contributor

Good point, thanks! I've addressed this in b5d5770.

chrislo added some commits Jan 3, 2018

Include hidden files when migrating to Asset Manager
See alphagov/asset-manager#368

We noticed that some assets in Whitehall have file names which include
a leading dot[1]. These files are treated as "hidden" and not
included in the array returned by `Dir.glob` by default.

This commit ensures that they are included by passing the
`FNM_DOTMATCH` flag to `Dir.glob`[2].

[1] e.g. `/data/uploads/whitehall/clean/system/uploads/consultation_response_form_data/file/257/.doc_Response_form.docx`
[2] https://ruby-doc.org/core-2.2.4/Dir.html#method-c-glob
Make assertion of array contents more explicit
Before this commit we were implicitly testing that the file paths
array contained the correct elements by checking its size. This commit
uses `assert_same_elements` to make the assertions stronger and easier
to understand.

@chrislo chrislo force-pushed the migrate-hidden-files branch from ef9ddfa to b5d5770 Jan 4, 2018

@chrislo

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

@floehopper thanks for the review. I've addressed your comments about the commit message in eba3dde.

Rebasing and force-pushing before merging.

@chrislo chrislo merged commit 0fe66f8 into master Jan 4, 2018

1 check passed

continuous-integration/jenkins/branch This commit looks good
Details

@chrislo chrislo deleted the migrate-hidden-files branch Jan 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.