-
Notifications
You must be signed in to change notification settings - Fork 56
Fileinstall: Permit directories containing broken symlinks #2948
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was no prior unit testing at all for this module. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| # Copyright (C) British Crown (Met Office) & Contributors. | ||
| # This file is part of Rose, a framework for meteorological suites. | ||
| # | ||
| # Rose is free software: you can redistribute it and/or modify | ||
| # it under the terms of the GNU General Public License as published by | ||
| # the Free Software Foundation, either version 3 of the License, or | ||
| # (at your option) any later version. | ||
| # | ||
| # Rose is distributed in the hope that it will be useful, | ||
| # but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| # GNU General Public License for more details. | ||
| # | ||
| # You should have received a copy of the GNU General Public License | ||
| # along with Rose. If not, see <http://www.gnu.org/licenses/>. | ||
| # ----------------------------------------------------------------------------- | ||
|
|
||
| import hashlib | ||
| from pathlib import Path | ||
| import pytest | ||
|
|
||
| from metomi.rose.checksum import get_checksum | ||
|
|
||
| HELLO_WORLD = hashlib.md5(b'Hello World').hexdigest() | ||
| HELLO_JUPITER = hashlib.md5(b'Hello Jupiter').hexdigest() | ||
|
|
||
|
|
||
| @pytest.fixture(scope='module') | ||
| def checksums_setup(tmp_path_factory): | ||
| """provide some exemplars for checksum to work on.""" | ||
| tmp_path = tmp_path_factory.getbasetemp() | ||
| (tmp_path / 'foo').write_text('Hello World') | ||
| (tmp_path / 'bar').mkdir() | ||
| (tmp_path / 'bar/baz').write_text('Hello Jupiter') | ||
| (tmp_path / 'goodlink').symlink_to('foo') | ||
| (tmp_path / 'badlink').symlink_to('bad') | ||
| yield tmp_path | ||
|
|
||
|
|
||
| @pytest.fixture(scope='module') | ||
| def checksums_dir(checksums_setup): | ||
| yield ( | ||
| {a: (b, c) for a, b, c in get_checksum(str(checksums_setup))}, | ||
| checksums_setup | ||
| ) | ||
|
|
||
|
|
||
| def test_checksum_single_file(checksums_setup): | ||
| res = get_checksum(str(checksums_setup / 'foo'))[0][1] | ||
| assert res == HELLO_WORLD | ||
|
|
||
|
|
||
| def test_checksum_custom_checksum_function(checksums_setup): | ||
| res = get_checksum( | ||
| str(checksums_setup / 'foo'), | ||
| # Last 3 letters of the reversed filename: | ||
| checksum_func=lambda x, _: x[-1:-4:-1] | ||
| )[0][1] | ||
| assert res == 'oof' | ||
|
|
||
|
|
||
| def test_get_checksum_for_all_files(checksums_dir): | ||
| assert len(checksums_dir[0]) == 7 | ||
|
|
||
|
|
||
| def test_get_checksum_for_goodlink(checksums_dir): | ||
| assert checksums_dir[0]['goodlink'][0] == HELLO_WORLD | ||
|
|
||
|
|
||
| def test_get_checksum_for_badlink(checksums_dir): | ||
| assert checksums_dir[0]['badlink'][0] == str(checksums_dir[1] / 'bad') | ||
|
|
||
|
|
||
| def test_get_checksum_for_subdir_file(checksums_dir): | ||
| assert checksums_dir[0]['bar/baz'][0] == HELLO_JUPITER | ||
|
|
||
|
|
||
| def test_get_checksum_for_non_path(): | ||
| unlikely = '/var/tmp/ariuaibvnoijunhoiujoiuj' | ||
| assert not Path(unlikely).exists() | ||
| with pytest.raises(FileNotFoundError, match=unlikely): | ||
| get_checksum(unlikely) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,6 +116,11 @@ root directory to install file targets with a relative path: | |
| ``~/.ssh/config`` to specify the user ID for logging into ``HOST`` | ||
| if required. | ||
|
|
||
| .. note:: | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO this could probably use a larger re-write, but simply getting this bit of info into the docs is a start. |
||
|
|
||
| * When installing files, broken symbolic links are rejected. | ||
| * When installing directories containing broken symlinks | ||
| they will be copied. | ||
|
|
||
| .. rose:conf:: file:TARGET | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| #!/usr/bin/env bash | ||
| #------------------------------------------------------------------------------- | ||
| # Copyright (C) British Crown (Met Office) & Contributors. | ||
| # | ||
| # This file is part of Rose, a framework for meteorological suites. | ||
| # | ||
| # Rose is free software: you can redistribute it and/or modify | ||
| # it under the terms of the GNU General Public License as published by | ||
| # the Free Software Foundation, either version 3 of the License, or | ||
| # (at your option) any later version. | ||
| # | ||
| # Rose is distributed in the hope that it will be useful, | ||
| # but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| # GNU General Public License for more details. | ||
| # | ||
| # You should have received a copy of the GNU General Public License | ||
| # along with Rose. If not, see <http://www.gnu.org/licenses/>. | ||
| #------------------------------------------------------------------------------- | ||
| # Test "rose app-run", file installation will not fail if a broken | ||
| # symlink is contained in a repository or directory. | ||
| # See https://github.com/metomi/rose/issues/2946 | ||
|
|
||
| . "$(dirname "$0")/test_header" | ||
|
|
||
| tests 4 | ||
|
|
||
| test_init <<__CONFIG__ | ||
| [command] | ||
| default=true | ||
|
|
||
| [file:destination] | ||
| source=${TEST_DIR}/source | ||
| __CONFIG__ | ||
|
|
||
| # Create a broken symlink dir | ||
| mkdir -p ${TEST_DIR}/source/ | ||
| touch ${TEST_DIR}/source/missing | ||
| ln -s ${TEST_DIR}/source/missing ${TEST_DIR}/source/link | ||
| rm ${TEST_DIR}/source/missing | ||
|
|
||
| test_setup | ||
|
|
||
| # It doesn't fail when rsync installs broken symlink dirs: | ||
| run_pass "${TEST_KEY_BASE}-rsync" rose app-run --config=../config | ||
| file_grep_fail "${TEST_KEY_BASE}-rsync.err" \ | ||
| "No such file" \ | ||
| "${TEST_KEY_BASE}-rsync.err" | ||
|
|
||
| # Turn the source file into a Git repo and try to use the git file-handler: | ||
| git -C ${TEST_DIR}/source init | ||
| git -C ${TEST_DIR}/source add . | ||
| git -C ${TEST_DIR}/source commit -a -m "my_commit" | ||
| sed -i 'sXsource=.*Xsource=git:../source::./::HEADX' ../config/rose-app.conf | ||
|
|
||
| # It doesn't fail when git installs broken symlink dirs: | ||
| run_pass "${TEST_KEY_BASE}-git" rose app-run --config=../config | ||
| file_grep_fail "${TEST_KEY_BASE}-git.err" \ | ||
| "No such file" \ | ||
| "${TEST_KEY_BASE}-git.err" | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally there should be an SVN test here too. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change targets the
get_checksumfunction in general, not the git / rsync methods specificity.Are we sure this method is only called for these methods, or should we pass the method name through to this function to ensure this logic is only activated when intended?
Also, this impacts Git, does it also impact SVN?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it makes sense to be consistent. The only other place this logic is used is Rose arch. I think it makes sense there too.
I hope so - I would have tested it (and still can if you insist) but setting up the test would have taken extra time.
@dpmatthews - what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should (and does) apply to all cases including SVN (I've tested this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oliver-sanders please resolve if you're happy...