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

Upload cucumber logs to S3 #8791

Merged
merged 4 commits into from Jun 7, 2016
Merged

Upload cucumber logs to S3 #8791

merged 4 commits into from Jun 7, 2016

Conversation

@islemaster
Copy link
Member

islemaster commented Jun 7, 2016

Puts cucumber logs in a versioned S3 bucket, divided into test and development environments, and updates logged URLs to point to specific versions on S3 so that past logs are still accessible. The S3 bucket is configured to keep noncurrent logs for 30 days.

Not yet tested on test - only on local, where it works as expected.

Puts cucumber logs in a versioned S3 bucket, divided into test and development environments, and updates logged URLs to point to specific versions on S3 so that past logs are still accessible.  The S3 bucket is configured to keep noncurrent logs for 30 days.
@@ -25,6 +26,35 @@

ENV['BUILD'] = `git rev-parse --short HEAD`

S3_LOGS_BUCKET = 'cucumber-logs'
S3_LOGS_PREFIX = rack_env?(:test) ? 'test' : 'development'

This comment has been minimized.

Copy link
@islemaster

islemaster Jun 7, 2016

Author Member

I'm open to suggestions on a better way to split this out; maybe I should just use hostname? I really want everything that runs on the test machine (re-runs, etc) to get bucketed together under test/ but for people doing UI tests locally to get their own bucket.

Oh! Or branch name. That wouldn't be too bad.

This comment has been minimized.

Copy link
@bcjordan

bcjordan Jun 7, 2016

Contributor

Ooh yeah, branch name could be nice. There is a GitUtils.current_branch

# Upload the given filename (of a cucumber log) to the logs s3 bucket.
# Returns a public URL to the uploaded log.
#
def upload_log_to_s3(filename, s3_client)

This comment has been minimized.

Copy link
@islemaster

islemaster Jun 7, 2016

Author Member

Question: Is this worth extracting into its own file/module yet? A PublicLogUploader of sorts?

This comment has been minimized.

Copy link
@bcjordan

bcjordan Jun 7, 2016

Contributor

Great idea, definitely support extracting this logic from runner.rb. Worth noting there is a utility S3.upload_to_bucket https://github.com/code-dot-org/code-dot-org/blob/staging/lib/cdo/aws/s3.rb#L43-L43 which grabs a new S3 client and uploads a file (but does not return the version #, so might not be perfect for this use case). That might be a good module to throw such a utility, though.

@bcjordan

This comment has been minimized.

Copy link
Contributor

bcjordan commented Jun 7, 2016

LGTM!

islemaster added 3 commits Jun 7, 2016
Mostly.  I've left the behavior around skipping logging if the --html option is omitted, and HipChat-logging errors, in runner.rb.  The actual S3 interactions have been moved to lib/cdo/aws/s3.rb though, which isn't a lot better but will probably make refactoring easier in the future.
@@ -9,6 +9,8 @@
require 'bundler'
require 'bundler/setup'

require 'cdo/aws/s3'
require 'cdo/git_utils'

This comment has been minimized.

Copy link
@bcjordan

bcjordan Jun 7, 2016

Contributor

No reason to expect it would, but in manual runs, the startup time of runner.rb didn't feel qualitatively longer including these libs, right?

This comment has been minimized.

Copy link
@islemaster

islemaster Jun 7, 2016

Author Member

I didn't get that impression.

@bcjordan

This comment has been minimized.

Copy link
Contributor

bcjordan commented Jun 7, 2016

LGTM!

@islemaster islemaster merged commit 0ae0e16 into staging Jun 7, 2016
3 checks passed
3 checks passed
ci/circleci Your tests passed on CircleCI!
Details
coverage/coveralls Coverage remained the same at 87.926%
Details
hound No violations found. Woof!
@islemaster islemaster deleted the cucumber-logs-on-s3 branch Jun 7, 2016
deploy-code-org added a commit that referenced this pull request Jun 7, 2016
2230104 Merge pull request #8780 from code-dot-org/npm-piskel (Brad Buchanan)
69e5430 Merge branch 'test' into staging (Josh Lory)
d9c4601 Merge branch 'production' into test (Josh Lory)
a895055 Merge pull request #8811 from code-dot-org/test (Josh Lory)
0585f54 Merge pull request #8783 from code-dot-org/pcardune-content-security-policy (Paul Carduner)
e873ec7 Merge pull request #8786 from code-dot-org/pcardune-apps-coverage-report (Paul Carduner)
2d57ffd Merge pull request #8809 from code-dot-org/revert-8748-firebase-basics (David Bailey)
0e85abc Revert "Firebase basics" (David Bailey)
bdf3fbb Merge pull request #8784 from code-dot-org/hide_solution_link_for_pd (Mehal Shah)
c274d51 Merge pull request #8808 from code-dot-org/hotfix-progress-dots (Josh Lory)
3fda3ff Hotfix: fix unplugged progress dots rendering issue (Josh Lory)
e1bcd99 Amend 799c13c (Josh Lory)
bfde939 Merge pull request #8787 from code-dot-org/bounce-ui-tests (David Bailey)
460c2d6 Merge pull request #8748 from code-dot-org/firebase-basics (David Bailey)
85bfdce Merge pull request #8794 from code-dot-org/fixPrivilegesUI (ashercodeorg)
fa1d792 Merge pull request #8788 from code-dot-org/fix-test-iphone-hocReset (Andrew Oberhardt)
0ae0e16 Merge pull request #8791 from code-dot-org/cucumber-logs-on-s3 (Brad Buchanan)
d36fa89 Merge pull request #8806 from code-dot-org/test (Josh Lory)
a644205 PR feedback - going back to using forbidden (Mehal Shah)
fc95f09 Merge pull request #8804 from code-dot-org/staging (Josh Lory)
c373dff Merge pull request #8803 from code-dot-org/progress-dots-fix (Josh Lory)
799c13c Unplugged dots should be small when not the current level (Josh Lory)
9baccd6 Merge pull request #8801 from code-dot-org/localsLevel (Brent Van Minnen)
6826eeb locals.level instead of just level (Brent Van Minnen)
5d82aee Merge remote-tracking branch 'origin/staging' into cucumber-logs-on-s3 [ui test] (Brad Buchanan)
267321f Extract log uploader from runner.rb (Brad Buchanan)
5a59be7 Merge pull request #8800 from code-dot-org/staging (Brent Van Minnen)
436eee6 Use branchname as S3 prefix for log (Brad Buchanan)
ce5fd7a Merge pull request #8797 from code-dot-org/pcardune-fix-netsim (Brent Van Minnen)
d48a4ad Use locals to refer to variables passed in (Paul Carduner)
1341eda Merge pull request #8756 from code-dot-org/react-progress-inline-styles (Josh Lory)
f28869e Load Piskel via nonfingerprinted path (Brad Buchanan)
6d6e7d0 Code review feedback [ci skip] (Josh Lory)
76ad952 Merge pull request #8796 from code-dot-org/levelbuilder (Josh Lory)
e53f5c2 Level builder changes (Continuous Integration)
5e53305 Add some more documentation (Paul Carduner)
@@ -25,6 +27,26 @@

ENV['BUILD'] = `git rev-parse --short HEAD`

S3_LOGS_BUCKET = 'cucumber-logs'
S3_LOGS_PREFIX = GitUtils.current_branch

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Jun 7, 2016

Contributor

Great improvement Brad! In the case where a developer runs a local ./runner.rb against the test server, it looks like this will use the developer's local branch name as the bucket. Perhaps it would be better to use the remote server name as the prefix in the case where it's not a local run?

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Jun 7, 2016

Contributor

Realized just after sending this: my main concern is that we don't pollute the logs for the "test" branch with local runs. So, maybe this is not a problem, unless developers frequently check out a branch locally named "test", which seems unlikely.

This comment has been minimized.

Copy link
@islemaster

islemaster Jun 7, 2016

Author Member

I didn't consider the case where ./runner.rb is being used to test a remote machine running a different branch than the local machine has checked out.

I thought about prefixing logs <hostname>/<branchname> to cover cases like this (local developer has staging checked out but is running against test machine, local developer has test checked out but is running against local changes, etc) so that only the test machine would be dropping stuff into the test/test prefix.

This would be easy to switch to, but based on our usual workflow I don't expect it to become an issue. I also wasn't sure we wanted all of our machine hostnames in public log URLs.

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