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

CUMULUS-1748: support relative paths in recursion.js #1485

Merged
merged 64 commits into from
Feb 22, 2020
Merged

Conversation

nemreid
Copy link
Contributor

@nemreid nemreid commented Feb 19, 2020

Summary: Summary of changes

Addresses CUMULUS-1748: relative paths in recursion

Changes

  • Support for relative paths
  • Also fix CUMULUS-1750

PR Checklist

  • Update CHANGELOG
  • Unit tests
  • Adhoc testing
  • Integration tests

packages/ingest/recursion.js Outdated Show resolved Hide resolved
packages/ingest/util.js Show resolved Hide resolved
packages/ingest/test/test-FtpProviderClientFailure.js Outdated Show resolved Hide resolved
@Jkovarik Jkovarik self-requested a review February 19, 2020 16:02
Copy link
Contributor

@markdboyd markdboyd left a comment

Choose a reason for hiding this comment

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

Mostly comments/questions but a few requested changes

@@ -45,10 +45,11 @@ while ! $docker_command 'curl --connect-timeout 5 -sS -o /dev/null http://127.0
done
echo 'HTTP service is available'

$docker_command "mkdir /keys;cp $UNIT_TEST_BUILD_DIR/packages/test-data/keys/ssh_client_rsa_key /keys/; chmod -R 400 /keys"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the idea behind making it a root directory (/keys) instead of in the test-data directory to avoid it being crawled by the FTP discovery tests and having issues due to directory permissions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is both that and the fact that SFTP requires the key permissions to be 400.
Changing the way we react to running into a directory we can't access seemed out of scope for this PR but would be required for everything not to bomb out on encountering a directory that does not allow reads.

packages/ingest/FtpProviderClient.js Show resolved Hide resolved
packages/ingest/FtpProviderClient.js Outdated Show resolved Hide resolved
packages/ingest/FtpProviderClient.js Outdated Show resolved Hide resolved
packages/ingest/FtpProviderClient.js Show resolved Hide resolved
packages/ingest/test/test-FtpProviderClientFailure.js Outdated Show resolved Hide resolved
packages/ingest/test/test-recursion.js Show resolved Hide resolved
packages/ingest/test/test-recursion.js Outdated Show resolved Hide resolved
packages/ingest/util.js Show resolved Hide resolved
packages/sftp-client/index.js Show resolved Hide resolved
@nemreid nemreid merged commit 09ce287 into master Feb 22, 2020
@nemreid nemreid deleted the CUMULUS-1748 branch February 22, 2020 00:58
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.

3 participants