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 Oracle Coherence storage driver #920

Merged
merged 13 commits into from
Aug 21, 2023
Merged

Add Oracle Coherence storage driver #920

merged 13 commits into from
Aug 21, 2023

Conversation

tmiddlet2666
Copy link
Contributor

@tmiddlet2666 tmiddlet2666 commented Aug 9, 2023

This pull request aims to add Coherence implementation into storage.
Thanks for the review, Please let me know of any issues to resolve.

@gaby
Copy link
Member

gaby commented Aug 9, 2023

@tmiddlet2666 Thanks for the PR, lets us know when its ready for review

@gaby gaby changed the title Add Coherence implementation into storage Add Oracle Coherence storage driver Aug 9, 2023
@tmiddlet2666
Copy link
Contributor Author

@tmiddlet2666 Thanks for the PR, lets us know when its ready for review

Thanks, not sure why the test is vailing on downloading our coherence-ce image https://github.com/gofiber/storage/actions/runs/5804095386/job/15734559701?pr=920

Getting Error: Process completed with exit code 56.

The image is around 257MB, so reasonably large. Are there any limits on disk or downloads?

@gaby
Copy link
Member

gaby commented Aug 9, 2023

@tmiddlet2666 Thanks for the PR, lets us know when its ready for review

Thanks, not sure why the test is vailing on downloading our coherence-ce image https://github.com/gofiber/storage/actions/runs/5804095386/job/15734559701?pr=920

Getting Error: Process completed with exit code 56.

The image is around 257MB, so reasonably large. Are there any limits on disk or downloads?

250MB is not large by any means. The problem is something on that health/start script. Have you tried running that shell code locally?

coherence/coherence.go Outdated Show resolved Hide resolved
coherence/coherence.go Outdated Show resolved Hide resolved
coherence/coherence.go Outdated Show resolved Hide resolved
@gaby
Copy link
Member

gaby commented Aug 10, 2023

Adding TLS issue for refenrece here: oracle/coherence-go-client#47

@tmiddlet2666 Overall it looks good so far, do you want to add go1.21 to the CI now that it was released yesterday?

@tmiddlet2666
Copy link
Contributor Author

Thanks @gaby
I have added the Config options and currently they set env variables, but when the above issue in coherence-go-client is fixed and released, i can update this package.
Will add go 1.21.

@gaby
Copy link
Member

gaby commented Aug 10, 2023

Thanks @gaby I have added the Config options and currently they set env variables, but when the above issue in coherence-go-client is fixed and released, i can update this package. Will add go 1.21.

For reference for the Redis driver, we allow the user to provide a tls.Config object. That way they can customize it to their needs.

https://github.com/gofiber/storage/blob/main/redis/redis.go#L50C4-L50C4

Example:

// Create a client with support for TLS
cer, err := tls.LoadX509KeyPair("./client.crt", "./client.key")
if err != nil {
	log.Println(err)
	return
}
tlsCfg := &tls.Config{
	MinVersion:               tls.VersionTLS12,
	InsecureSkipVerify:       true,
	Certificates:             []tls.Certificate{cer},
}

@tmiddlet2666 tmiddlet2666 marked this pull request as ready for review August 15, 2023 01:57
@tmiddlet2666
Copy link
Contributor Author

@gaby - have moved from pending to proper PR

@gaby
Copy link
Member

gaby commented Aug 15, 2023

@gaby - have moved from pending to proper PR

Thanks for the contribution, will start reviewing this week. So far it looks good.

@gaby gaby requested a review from ReneWerner87 August 15, 2023 02:29
@gaby gaby requested a review from efectn August 15, 2023 02:29
Copy link
Member

@gaby gaby left a comment

Choose a reason for hiding this comment

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

lgtm, thank you!

@ReneWerner87
Copy link
Member

ReneWerner87 commented Aug 20, 2023

@tmiddlet2666 @gaby
image
what is the best thing to do in this case?

@gaby
Copy link
Member

gaby commented Aug 20, 2023

@tmiddlet2666 @gaby image what is the best thing to do in this case?

It was passing fine, and with the last gosec/go update it now gets flag.

@tmiddlet2666 We may need this to be solved: oracle/coherence-go-client#49

@tmiddlet2666
Copy link
Contributor Author

@tmiddlet2666 @gaby image what is the best thing to do in this case?

Let me work on adding the *tls.Config support.
I'll need to release a new go client.
will take me a couple of days and i'll ping you.
Thanks

@gaby
Copy link
Member

gaby commented Aug 21, 2023

@tmiddlet2666 @gaby image what is the best thing to do in this case?

Let me work on adding the *tls.Config support. I'll need to release a new go client. will take me a couple of days and i'll ping you. Thanks

Sounds like a plan, thank you!

@ReneWerner87 ReneWerner87 merged commit ff142d9 into gofiber:main Aug 21, 2023
10 checks passed
@tmiddlet2666 tmiddlet2666 deleted the coherence branch August 21, 2023 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants