Skip to content

Conversation

bkeepers
Copy link
Collaborator

@bkeepers bkeepers commented Dec 30, 2023

$ bundle add flipper-active_record
$ rails generate flipper:setup -t <YOUR-TOKEN>
      create  db/migrate/20231230170601_create_flipper_tables.rb
      append  config/credentials.yml.enc
  • Call flipper:active_record if adapter is installed
  • --token xyz adds to dotenv if it exists
  • --token xyz adds to Rails credentials if it it's used

Copy link
Collaborator

@garrettdimon garrettdimon left a comment

Choose a reason for hiding this comment

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

Overall, I dig it, but I do think the token should just be an argument since there's never a case where it would be run without it, right?

Also, not important for a first pass, but it might be nice to have an option to automatically run the migrations as well after generating them. It doesn't need to by default, but it reduces the amount of commands to type/copy/paste if we can have a sample command that includes the option and runs the migrations by default. (Although that does blur the line with whether this is generating or installing, so it may be a step too far.)

end

def configure_with_credentials
return unless exists?("config/credentials.yml.enc") && (ENV["RAILS_MASTER_KEY"] || exists?("config/master.key"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's common enough that the credentials can be split over individual environment files as well since that can help reduce conflicts when merging the encrypted files. So is it worth checking for production.yml.enc as well?

That question then leads to whether we'd want to proactively help set up multiple environments. So should the keys maybe be specified by environment? This all quickly complicates things, though.

I think I'd make the token an argument and then add an option for --environment that accepts a string and has --env (and -e if no conflicts) as aliases. Whether it should default to prod or dev is less immediately obvious to me at the moment, though. Dev gets things up and running faster, but production is more of the end game. I think I lean towards development.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, good call. I was only thinking about development on this first pass, but I'd like to iterate on this and add multiple environment support.

@bkeepers
Copy link
Collaborator Author

…but I do think the token should just be an argument since there's never a case where it would be run without it, right?

Open source users will run it without the arg, but I guess it could be optional. You and I also talked about allowing signup through the generator, like rails g flipper:setup [email protected], or fetching the token if given credentials. What do you think about waiting until we get some of the other functionality built to decide if this should be an arg instead of an option?

@garrettdimon
Copy link
Collaborator

I'm totally down with punting, I was just trying to minimize the churn for the generator arguments/options.

Copy link
Collaborator

@jnunemaker jnunemaker left a comment

Choose a reason for hiding this comment

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

Sweet! Love the reduction in friction. Does it also run the migrations after they are generated?

@bkeepers
Copy link
Collaborator Author

bkeepers commented Jan 8, 2024

Does it also run the migrations after they are generated?

It does not. Do other generators often run migrations? We definitely could do it, just not sure I've ever seen another library do that.

In all honesty, the other reason it doesn't is because I couldn't figure out a way to get the tests to still run with it. Adding rails_command 'db:migrate' blows up because we don't have a dummy rails app that the test are running against.

@bkeepers bkeepers merged commit b27ba80 into main Jan 8, 2024
@bkeepers bkeepers deleted the setup-generator branch January 8, 2024 14:36
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