-
Notifications
You must be signed in to change notification settings - Fork 106
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
add http/ftp/sftp support to circleci builds #226
Conversation
@@ -3,7 +3,7 @@ version: 2 | |||
jobs: | |||
build_and_test: | |||
docker: | |||
- image: circleci/node:6.10 | |||
- image: cumuluss/circleci:node-6.10 |
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.
Should this be cumulus
rather than cumuluss
?
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.
ok, just looked at docker hub and the answer is no, cumuluss
is not a typo but dang it looks like one. Is that meant to be a temporary account?
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.
unfortunately, cumulus
namespace is taken on docker. We are using cumuluss
for now :(
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.
Wouldn't multiple cumulus items be "cumuli"?
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.
@yjpa7145 great point. @sethvincent do we have cumuli
?
recursivelyDeleteS3Bucket(internalBucketName), | ||
fs.remove(providerPathDirectory) | ||
]); | ||
await recursivelyDeleteS3Bucket(internalBucketName); |
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 this could be DRY'ed with a test.after.always
block with line 110 and we wouldn't need these finally blocks
recursivelyDeleteS3Bucket(internalBucketName), | ||
fs.remove(providerPathDirectory) | ||
]); | ||
await recursivelyDeleteS3Bucket(internalBucketName); |
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.
same comment about test.after.always
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.
@abarciauskas-bgse these tests are from Marc's work. I just simplified them here to get the SFTP work. I rather not make more changes to these tests for now.
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.
@abarciauskas-bgse or @scisco If this isn't going to be fixed now, can you create a ticket so that we can come back to it?
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.
Couple nits, but otherwise LGTM
Summary: Summary of changes
Addresses CUMULUS-354
Changes
packages/test-data
(which enables us to use SFTP inside the circleci environment)