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

Support non-linear phase dependencies #122

Merged
merged 7 commits into from
Jan 23, 2025
Merged

Support non-linear phase dependencies #122

merged 7 commits into from
Jan 23, 2025

Conversation

yevhenii-ponomarenko
Copy link
Contributor

feat: support non-linear phase dependencies

Checklist:

  • I have updated the necessary documentation
  • I have signed off all my commits as required by DCO
  • My build is green

Author: James Dabbs <[email protected]>
Date:   Thu Jan 2 13:45:38 2025 -0800

    feat: support non-linear phase dependencies

Signed-off-by: Yevhenii Ponomarenko <[email protected]>
@yevhenii-ponomarenko yevhenii-ponomarenko requested review from a team as code owners January 7, 2025 11:57
@yevhenii-ponomarenko yevhenii-ponomarenko changed the title Introduce Phases Support non-linear phase dependencies Jan 7, 2025
spec/dummy/spec/handcuffs_rake_spec.rb Outdated Show resolved Hide resolved
Comment on lines +82 to +84
# Prevent running post_restart migrations if there are outstanding
# pre_restart migrations
post_restart: [:pre_restart],
Copy link
Contributor

Choose a reason for hiding this comment

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

nb: Even though this is the only additional configuration at the moment, it might worth an additional refactor here to make this more explicit:

Handcuffs.configure do |config|
  phases << Phase.new(:pre_restart)
  phases << Phase.new(:post_restart, prerequisites: [:pre_restart]
end

# Or obsfuscate the initialization.
Handcuffs.configure do |config|
  config.phases = [
    :pre_restart,
    { post_restart: { prerequisites: [:pre_restart]}
  ]
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is sort of where I started, then pared it back a bit. But another thing we want at some point is phase aliases (s.t. we can gradually do simplifying renames), and at that point, you sort of need something like this. Happy to take a swing at adding that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the explicitness, but the notation gets more complicated and messy, and potentially more intricate parsing and validation as well.
In order to make the current simple config more usable by shortening the feedback loop, I added a rake task that both validates and prints out what Handcuffs perceives to the be ordering and dependencies, so if there is any question in the end users mind they can all ways do a quick check and modify their configuration as needed.

Copy link
Contributor

@kkohrt kkohrt left a comment

Choose a reason for hiding this comment

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

All for this addition. Some suggestions below on minor changes that could be made here or in a follow up.

attr_accessor :default_phase

def initialize
@phases = []
@default_phase = nil
end

def phases=(phases)
@phases = Phases.new(phases)
Copy link
Contributor

Choose a reason for hiding this comment

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

Although there was no format checking prior to this change, a simple array is hard to do incorrectly. But I wonder if it would make sense to evaluate the input now that we are providing the ability to define a graph via a hash. That is much more complex, and maybe better to flag issues at at configuration time rather than flag a configuration error when migrations are actually run.
If nothing else, the configuration class seems like the right place to provide meaningful feedback for invalid configurations.

Two failure cases I could see happening are
(1) Not defining dependencies as an array

phases = { a: [:b, :c], b: [], c: :b }
ph = Phases.new(phases) # <== no errors here
# running migrations leads to:
ph.in_order
(irb):26:in `block in in_order': undefined method `each' for :b:Symbol (NoMethodError)

        ->(phase, &block) { @phases.fetch(phase).each(&block) }

(2) accidentally defining a circular dependency (not likely in simple setups, but possible in more complex configurations)

phases = { a: [:b, :c], b: [:c], c: [:a] }
ph = Phases.new(phases) # <== no errors here
# running migrations leads to:
ph.in_order
(irb):24:in `in_order': uninitialized constant Phases::TSort (NameError)

      TSort.tsort(
      ^^^^^

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@kkohrt
Copy link
Contributor

kkohrt commented Jan 9, 2025

@jamesdabbs-procore & @yevhenii-ponomarenko - I added a rake task to check/validate the phase configuration and updated the README to (hopefully) be a little more clear about all of the moving parts.

If you'd like this PR would merge those changes into this branch
#123

kkohrt and others added 2 commits January 23, 2025 13:59
Co-authored-by: Kevin Kohrt <[email protected]>
Signed-off-by: Kevin Kohrt <[email protected]>
Signed-off-by: Jacob Sheehy <[email protected]>
Co-authored-by: Kevin Kohrt <[email protected]>
Signed-off-by: Kevin Kohrt <[email protected]>
Signed-off-by: Jacob Sheehy <[email protected]>
lessthanjacob
lessthanjacob previously approved these changes Jan 23, 2025
bmanuel
bmanuel previously approved these changes Jan 23, 2025
Copy link
Contributor

@kkohrt kkohrt left a comment

Choose a reason for hiding this comment

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

last few thoughts, below

README.md Outdated
end
```

The default phase order in this case is determined by [Tsort](https://github.com/ruby/tsort) (topilogical sort). In order to validate the configuration and expected phase order it is reccomended that you check the phase configuration after any changes using the rake task:
Copy link
Contributor

Choose a reason for hiding this comment

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

Typos (my bad)

Suggested change
The default phase order in this case is determined by [Tsort](https://github.com/ruby/tsort) (topilogical sort). In order to validate the configuration and expected phase order it is reccomended that you check the phase configuration after any changes using the rake task:
The default phase order in this case is determined by [Tsort](https://github.com/ruby/tsort) (topological sort). In order to validate the configuration and expected phase order it is recommended that you check the phase configuration after any changes using the rake task:

README.md Show resolved Hide resolved
@@ -1,7 +1,7 @@
PATH
remote: ../../..
specs:
handcuffs (1.4.1)
handcuffs (2.0.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should end up at

Suggested change
handcuffs (2.0.0)
handcuffs (2.1.0)

by the time we tag it.
But that can be in a followup PR for the actual version bump, if we want. (A PR that would finalize the release date in the CHANGELOG, too)

Signed-off-by: Jacob Sheehy <[email protected]>
@lessthanjacob lessthanjacob dismissed stale reviews from bmanuel and themself via 1970150 January 23, 2025 23:03
@bmanuel bmanuel merged commit 7413466 into main Jan 23, 2025
23 checks passed
@bmanuel bmanuel deleted the mobby/james_phases branch January 23, 2025 23:10
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.

5 participants