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

feat: new GO Feature Flag ruby provider #38

Merged
merged 19 commits into from
Aug 13, 2024
Merged

Conversation

thomaspoignant
Copy link
Member

This PR

  • This PR contains the implementation of a ruby provider for GO Feature Flag.

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>
@thomaspoignant thomaspoignant changed the title feat: Init GO Feature Flag ruby provider feat: new GO Feature Flag ruby provider Aug 5, 2024
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>
Copy link
Contributor

@alxckn alxckn left a 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 :)

Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Copy link
Member

@maxveldink maxveldink left a 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/README.md Outdated Show resolved Hide resolved
providers/openfeature-go-feature-flag-provider/README.md Outdated Show resolved Hide resolved
providers/openfeature-go-feature-flag-provider/README.md Outdated Show resolved Hide resolved
Comment on lines +5 to +18
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
Copy link
Member

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.

Suggested change
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!

Copy link
Member Author

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.

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>
Copy link
Member Author

@thomaspoignant thomaspoignant left a 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,

Comment on lines +5 to +18
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
Copy link
Member Author

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.

@thomaspoignant thomaspoignant merged commit a0bbf53 into main Aug 13, 2024
11 checks passed
@thomaspoignant thomaspoignant deleted the goff-ruby-provider branch August 13, 2024 14:46
thomaspoignant pushed a commit that referenced this pull request Aug 13, 2024
🤖 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>
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.

3 participants