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

Read only #92

Closed
wants to merge 2 commits into from
Closed

Read only #92

wants to merge 2 commits into from

Conversation

dangerp
Copy link

@dangerp dangerp commented Dec 4, 2015

Adds a #read_only accessor on DSL and Feature, as well as an initialization option. When set, any attempts to enable or disable features raises an error.

A common use case is where a separate application (using flipper-ui) is managing the feature flags.

@jnunemaker
Copy link
Collaborator

Awesome. Thanks for opening this up. I've been thinking about it for a few minutes and I'm on the fence. Did you consider any alternatives or go straight to this? Just curious as I'm wondering if you had any more context.

One thought I had is maybe this should be pushed to the adapter level. I then thought the downside of that is that each adapter would have to implement it. I then realized that we wouldn't have to do that. Instead, we could just create a read only adapter that wraps any adapter of your choosing (similar to the memoizable and operation logging (https://github.com/jnunemaker/flipper/blob/9ea8a7df5aad222bd3332f09c082071a1898c835/lib/flipper/adapters/operation_logger.rb#L8) adapters). Doing this would allow zero changes to flipper (other than perhaps adding the read only adapter and docs), yet support your use case. What are your thoughts on that?

It could probably look like this:

require 'flipper/adapters/decorator'

module Flipper
  module Adapters
    class ReadOnly < Decorator
      class ReadOnlyMode < Error
        def initialize(message = nil)
          super(message || "write attempted while in read only mode")
        end
      end

      def initialize(adapter)
        super(adapter)
      end

      def add(feature)
        raise ReadOnlyMode 
      end

      def remove(feature)
        raise ReadOnlyMode
      end

      def clear(feature)
        raise ReadOnlyMode
      end

      def enable(feature, gate, thing)
        raise ReadOnlyMode
      end

      def disable(feature, gate, thing)
        raise ReadOnlyMode
      end
    end
  end
end

And be used like this:

memory_adapter = Flipper::Adapters::Memory.new
readonly_memory_adapter = Flipper::Adapters::ReadOnly.new(memory_adapter)
flipper = Flipper.new(readonly_memory_adapter)

Does this seem like it would get the job done for you in a good enough way? I tend to prefer a proxy like this to adding an option that has to be maintained throughout the library.

@dangerp
Copy link
Author

dangerp commented Dec 4, 2015

I tried a couple options before resorting to making changes to flipper itself, but the only options seemed to be either implementing a new adapter or adding the option to flipper. However, I do like your wrapper suggestion, it's a lot cleaner than having to pass an option down through multiple layers.

I'll try out your implementation with some tests.

@jnunemaker
Copy link
Collaborator

@dangerp awesome. If you have any issues or run out of time let me know and I can try to put something together.

jnunemaker added a commit that referenced this pull request Mar 4, 2016
@jnunemaker jnunemaker mentioned this pull request Mar 4, 2016
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.

2 participants