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

Allow templating in config values #32

Merged
merged 6 commits into from
Mar 30, 2021
Merged

Allow templating in config values #32

merged 6 commits into from
Mar 30, 2021

Conversation

acroos
Copy link
Contributor

@acroos acroos commented Mar 28, 2021

Description

It's likely that users will want values for some configuration options that are unique per migration, or at least scoped to the table that's being migrated (for example: the flag files). This can now be solved by using ERB like:

config.ghost_adapter.serve_socket_file = '/tmp/<%= table %>/<%= pid %>-<%= timestamp %>.sock'

Any configuration value can be templated into another config value, and additionally the following values are always available:

  • pid: the current process ID
  • timestamp: current seconds since epoch (as integer)
  • unique_id: random UUID
  • table: the table being migrated
  • database: the database the migration is being run against

Addresses half of #23

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

New unit tests added to check functionality. Also tested with a demo rails application (ruby 2.7)

Checklist:

  • My code follows the style guidelines set by rubocop
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@acroos acroos self-assigned this Mar 28, 2021
@acroos acroos requested a review from nickdowse March 28, 2021 23:36
@@ -92,21 +92,30 @@ def compact
to_h.compact
end

def as_args
with_env.map { |key, value| arg(key, value) }.compact
def as_args(context: {})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

starting to think the whole "args" logic should move from Config and into Command

Choose a reason for hiding this comment

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

I think that moving at least the ENV vars into the command section makes sense. At the moment understanding the priority order of the different ways to configure options is the most complex part of this library, it would be great to be able to simplify that, out of scope for this PR however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any chance you wanna make an issue for this and describe the desired outcome? I think you're on to something with ENV config not being merged into config within the config class, but instead the two being merged just-in-time when the command is formed.

Copy link

@nickdowse nickdowse left a comment

Choose a reason for hiding this comment

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

Nice, I like this work a lot. Agreed that it now looks like a good time to take a holistic look at how we manage options and see if there is a way we can simplify the layers there.

spec/ghost_adapter/config_spec.rb Outdated Show resolved Hide resolved
@@ -92,21 +92,30 @@ def compact
to_h.compact
end

def as_args
with_env.map { |key, value| arg(key, value) }.compact
def as_args(context: {})

Choose a reason for hiding this comment

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

I think that moving at least the ENV vars into the command section makes sense. At the moment understanding the priority order of the different ways to configure options is the most complex part of this library, it would be great to be able to simplify that, out of scope for this PR however.

@acroos acroos merged commit 06ddad5 into main Mar 30, 2021
@acroos acroos deleted the templated-configuration branch March 30, 2021 16:05
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