-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: new GO Feature Flag ruby provider #38
Conversation
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
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.
Looks pretty complete overall :)
...feature-go-feature-flag-provider/lib/openfeature/go-feature-flag/go_feature_flag_provider.rb
Outdated
Show resolved
Hide resolved
...feature-go-feature-flag-provider/lib/openfeature/go-feature-flag/go_feature_flag_provider.rb
Outdated
Show resolved
Hide resolved
providers/openfeature-go-feature-flag-provider/lib/openfeature/go-feature-flag/goff_api.rb
Show resolved
Hide resolved
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
92075ae
to
d29df64
Compare
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 don't see any real blockers to merging this in an releasing a 0.1.0
with this. I found it hard to believe this was your first real go at Ruby @thomaspoignant!
A few things:
- I would be interested in chopping this PR up a little bit more and merging in some smaller chunks that I could provide more in-depth feedback on: at least maybe starting with a scaffolding PR for the new gem structure, and separate PRs for implementing the Ruby-focused interaction with GO Feature Flag and the actual Provider wire up
- I think there are some extractable pieces here around generalizing an OFREP-compliant HTTP client that we could easily reuse in other providers that I'm interested in iterating on in the future
- I did not have too much time tor really dig into spec feedback or provide as many Ruby notes on the Provider itself; if we do end up chopping this into multiple PRs, I can provide more detailed feedback there.
providers/openfeature-go-feature-flag-provider/spec/spec_helper.rb
Outdated
Show resolved
Hide resolved
providers/openfeature-go-feature-flag-provider/spec/spec_helper.rb
Outdated
Show resolved
Hide resolved
providers/openfeature-go-feature-flag-provider/lib/openfeature/go-feature-flag/error/errors.rb
Outdated
Show resolved
Hide resolved
providers/openfeature-go-feature-flag-provider/lib/openfeature/go-feature-flag/error/errors.rb
Outdated
Show resolved
Hide resolved
providers/openfeature-go-feature-flag-provider/lib/openfeature/go-feature-flag/goff_api.rb
Outdated
Show resolved
Hide resolved
context "#endpoint" do | ||
it "should have a valid endpoint set" do | ||
options = OpenFeature::GoFeatureFlag::Options.new(endpoint: "http://localhost:1031") | ||
expect(options.endpoint).to eql("http://localhost:1031") | ||
end | ||
|
||
it "should raise if endpoint is invalid" do | ||
expect { OpenFeature::GoFeatureFlag::Options.new(endpoint: "invalid_url") }.to raise_error(ArgumentError, "Invalid URL for endpoint: invalid_url") | ||
end | ||
|
||
it "should raise if endpoint is not http" do | ||
expect { OpenFeature::GoFeatureFlag::Options.new(endpoint: "ftp://gofeatureflag.org") }.to raise_error(ArgumentError, "Invalid URL for endpoint: ftp://gofeatureflag.org") | ||
end | ||
end |
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.
suggestion(non-blocking): These are great specs for your first use of RSpec! Completely optional, but a handy paradigm to get into in RSpec is to use the context
blocks as scopes where you're altering the test world based on the conditionals in your test names. For example, we could write this test to highlight that the endpoint
variable is changing, utilizing contexts.
context "#endpoint" do | |
it "should have a valid endpoint set" do | |
options = OpenFeature::GoFeatureFlag::Options.new(endpoint: "http://localhost:1031") | |
expect(options.endpoint).to eql("http://localhost:1031") | |
end | |
it "should raise if endpoint is invalid" do | |
expect { OpenFeature::GoFeatureFlag::Options.new(endpoint: "invalid_url") }.to raise_error(ArgumentError, "Invalid URL for endpoint: invalid_url") | |
end | |
it "should raise if endpoint is not http" do | |
expect { OpenFeature::GoFeatureFlag::Options.new(endpoint: "ftp://gofeatureflag.org") }.to raise_error(ArgumentError, "Invalid URL for endpoint: ftp://gofeatureflag.org") | |
end | |
end | |
describe "#endpoint" do | |
subject(:options) { described_class.new(endpoint:) } | |
context "when endpoint is valid" do | |
let(:endpoint) { "http://localhost:1031" } | |
it "sets endpoint" do | |
expect(options.endpoint).to eq(endpoint) | |
end | |
end | |
context "when endpoint is invalid" do | |
let(:endpoint) { "invalid_url" } | |
it "raises ArgumentError" do | |
expect { options }.to raise_error(ArgumentError, "Invalid URL for endpoint: invalid_url") | |
end | |
end | |
context "when endpoint is non-http" do | |
let(:endpoint) { "ftp://gofeatureflag.org" } | |
it "raises ArgumentError" do | |
expect { options }.to raise_error(ArgumentError, "Invalid URL for endpoint: ftp://gofeatureflag.org") | |
end | |
end | |
end |
I'll avoid giving the same advice on the other tests, and I'll approve as is, but just some food for thought!
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.
interesting I will keep it in mind when writing new tests.
providers/openfeature-go-feature-flag-provider/lib/openfeature/go-feature-flag/goff_api.rb
Outdated
Show resolved
Hide resolved
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
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.
@maxveldink thanks a lot for your review.
I am not sure I will have the time to split this PR in multiple smaller ones (I am on vacation starting this week and I would like to merge it).
As discussed this 1st implementation could be a good base for an OFREP provider.
In the fixes I have addressed most of your comments.
The biggest one is that I now use faraday
as HTTP Client, which avoids a lot of boilerplate when it comes to configuring the HTTP connection,
providers/openfeature-go-feature-flag-provider/lib/openfeature/go-feature-flag/goff_api.rb
Outdated
Show resolved
Hide resolved
context "#endpoint" do | ||
it "should have a valid endpoint set" do | ||
options = OpenFeature::GoFeatureFlag::Options.new(endpoint: "http://localhost:1031") | ||
expect(options.endpoint).to eql("http://localhost:1031") | ||
end | ||
|
||
it "should raise if endpoint is invalid" do | ||
expect { OpenFeature::GoFeatureFlag::Options.new(endpoint: "invalid_url") }.to raise_error(ArgumentError, "Invalid URL for endpoint: invalid_url") | ||
end | ||
|
||
it "should raise if endpoint is not http" do | ||
expect { OpenFeature::GoFeatureFlag::Options.new(endpoint: "ftp://gofeatureflag.org") }.to raise_error(ArgumentError, "Invalid URL for endpoint: ftp://gofeatureflag.org") | ||
end | ||
end |
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.
interesting I will keep it in mind when writing new tests.
🤖 I have created a release *beep* *boop* --- ## [0.1.1](openfeature-go-feature-flag-provider-v0.1.0...openfeature-go-feature-flag-provider/v0.1.1) (2024-08-13) ### ✨ New Features * new GO Feature Flag ruby provider ([#38](#38)) ([a0bbf53](a0bbf53)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com>
This PR