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

Add s3 support, fix bug with chrome.go, fix readme curl example. #6

Merged
merged 6 commits into from Feb 19, 2019

Conversation

mdanzinger
Copy link
Contributor

Hey! Sorry for the delay in getting the PR your way. I've added an s3Object which implements your StoredFile interface. I've used a singleton pattern to ensure we're not creating new s3 clients on every new s3Object creation.

Code can probably be cleaned up / simplified - looking forward to whatever feedback you may have!

Thanks!

@mdanzinger mdanzinger mentioned this pull request Feb 3, 2019
renderer/chrome.go Outdated Show resolved Hide resolved
renderer/chrome.go Outdated Show resolved Hide resolved
storage/s3.go Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
storage/s3.go Outdated Show resolved Hide resolved
@vjiandani-r7
Copy link
Contributor

Looks good! Thanks for this. I'll wait until you give me the thumbs up to do a quick test locally. Assuming all is well, I'll approve/merge.

@mdanzinger
Copy link
Contributor Author

Thanks for taking the time to review this. I'll be implementing your changes this weekend.

@mdanzinger
Copy link
Contributor Author

@vjiandani-r7 Pushed changes as per your feedback.

storage/s3.go Outdated Show resolved Hide resolved
storage/s3.go Outdated Show resolved Hide resolved
cfg/environment.go Outdated Show resolved Hide resolved
@vjiandani-r7
Copy link
Contributor

@mdanzinger I noticed a couple more things.

@mdanzinger
Copy link
Contributor Author

Hey @vjiandani-r7, I've gone ahead and implemented your changes.

@vjiandani-r7
Copy link
Contributor

I'm getting an error when trying to render a pdf: Failed to render: https://google.com\n Error: cdp.Target: CreateBrowserContext: rpc error: Not allowed. (code = -32000) Looking but if you know, let me know.

@mdanzinger
Copy link
Contributor Author

@vjiandani-r7 That was the error I was receiving too, commit 67acb56 seemed to have fixed it for me. Are you running an updated version of chrome?

@vjiandani-r7
Copy link
Contributor

I figured that one out. My bad on how I had things set up. I am running into the following issue while running the unit tests though:

go test -v ./...
?   	github.com/rapid7/pdf-renderer	[no test files]
?   	github.com/rapid7/pdf-renderer/cfg	[no test files]
?   	github.com/rapid7/pdf-renderer/correlation	[no test files]
?   	github.com/rapid7/pdf-renderer/renderer	[no test files]
=== RUN   TestNewS3Client
time="2019-02-19T12:50:55-08:00" level=error msg="MissingRegion: could not find region configuration"
MissingRegion: could not find region configuration
--- FAIL: TestNewS3Client (0.00s)
=== RUN   TestNewS3Object
time="2019-02-19T12:50:55-08:00" level=error msg="MissingRegion: could not find region configuration"
MissingRegion: could not find region configuration
--- FAIL: TestNewS3Object (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x13caa57]

goroutine 6 [running]:
testing.tRunner.func1(0xc4201cc1e0)
	/usr/local/Cellar/go/1.10.3/libexec/src/testing/testing.go:742 +0x29d
panic(0x1438fa0, 0x1777c20)
	/usr/local/Cellar/go/1.10.3/libexec/src/runtime/panic.go:502 +0x229
github.com/rapid7/pdf-renderer/storage.(*s3Object).Write(0x0, 0xc42001eea0, 0x11, 0x20, 0x11, 0x20)
	/r7/src/go-projects/src/github.com/rapid7/pdf-renderer/storage/s3.go:105 +0x47
github.com/rapid7/pdf-renderer/storage.TestNewS3Object(0xc4201cc1e0)
	/r7/src/go-projects/src/github.com/rapid7/pdf-renderer/storage/s3__test.go:25 +0xfa
testing.tRunner(0xc4201cc1e0, 0x14e71d0)
	/usr/local/Cellar/go/1.10.3/libexec/src/testing/testing.go:777 +0xd0
created by testing.(*T).Run
	/usr/local/Cellar/go/1.10.3/libexec/src/testing/testing.go:824 +0x2e0
FAIL	github.com/rapid7/pdf-renderer/storage	0.075s
?   	github.com/rapid7/pdf-renderer/web	[no test files]
make: *** [test] Error 1

@mdanzinger
Copy link
Contributor Author

Do you have a region set in the ~/.aws/credentials file?

region = us-east-2

@vjiandani-r7
Copy link
Contributor

Ok I think this is fine. I'll probably want to add support for https://docs.aws.amazon.com/sdk-for-go/api/aws/ec2metadata/ though. Unless you would like to do that.

@vjiandani-r7 vjiandani-r7 merged commit 36e9577 into rapid7:master Feb 19, 2019
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.

None yet

2 participants