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

Smarter update diff simpler #184

Merged
merged 10 commits into from
Sep 15, 2022
Merged

Smarter update diff simpler #184

merged 10 commits into from
Sep 15, 2022

Conversation

zdrve
Copy link
Collaborator

@zdrve zdrve commented Sep 3, 2022

Have .plan and .update return useful data structures.

Unrelated, though useful for testing: allow the Managed by kennel marker to be overridden.

@zdrve zdrve marked this pull request as ready for review September 5, 2022 19:42
Kennel.out.puts "#{LINE_UP}Deleted #{message}"
end

Update.new(update_log: update_log)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that update_log exposes quite a bit of data (and all undocumented). For the purposes of the corresponding zendesk kennel PR we could have made it expose just the tracking ids.

Copy link
Owner

Choose a reason for hiding this comment

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

how is the update-log different from a Plan ? ... it has the ids resolved ?
can we use the Plan internally and then resolve the ids in the plan to have the same data structure at the end ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well.... at the moment, perhaps not much. My longer term view however is this:

The way that plan works at the moment – and therefore what the plan actually is – is very simplistic. I prefer to think of it not so much of a plan, but more of a diff; a comparison. If we compare A to B, then this is added, this is changed, this is removed.

If the job then is resolve those differences (assuming it's a non-empty set), then we need to do something. What, exactly? Well, that's the plan. Or, once performed, the results.

Our methodology is already showing its shortcomings. If we for example create a monitor M with tracking id K and type T, and then we update kennel so that the type now needs to be T', then (IIRC) Kennel currently can't handle that, because Datadog does not allow a monitor's type to be changed. However, it is solvable: instead of updating the monitor, delete monitor M then create monitor M' type T' (and with the same tracking ID K). In this case, the diff would be "Monitor K change T => T'", but the plan would be "delete M and create M'".

Another example: circular references. The reason that I haven't yet merged #160 is that it would allow for / increase the likelihood of circular references. Monitor M, whose message text includes a link to troubleshooting dashboard D. But dashboard D contains a view of monitor M. So if M => D and D => M, and neither exist, so the diff is "both D and M are new", what's the plan? Possible answer: create M (but with the message text only containing the kennel tracking text); create D, referring to M; update M to set its message text correctly, linking to D.

To return to your question, "what's the difference between the plan and the update log?". What we currently call plan is a diff. What I'm calling update_log is closer to a plan – but retrospective.

@zdrve zdrve marked this pull request as draft September 5, 2022 19:54
@zdrve zdrve marked this pull request as ready for review September 5, 2022 19:54
lib/kennel.rb Outdated Show resolved Hide resolved
lib/kennel.rb Show resolved Hide resolved
lib/kennel.rb Outdated Show resolved Hide resolved
lib/kennel.rb Outdated Show resolved Hide resolved
Comment on lines 35 to 36
noop?: noop?,
already_ok: @already_ok,
Copy link
Owner

Choose a reason for hiding this comment

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

these seem a bit redundant, maybe a plan class could hold the attrs and have a noop? method ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe. In a later refactor, I would suggest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're not suggesting changing the interface, just the implementation.

lib/kennel/syncer.rb Outdated Show resolved Hide resolved
Comment on lines 17 to 18
@environment ||= begin
require "kennel"
Copy link
Owner

Choose a reason for hiding this comment

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

maybe cleaner ?

Suggested change
@environment ||= begin
require "kennel"
return if @environment_loaded
@environment_loaded = true
require "kennel"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Less safe IMO

lib/kennel/tasks.rb Outdated Show resolved Hide resolved
@zdrve zdrve requested a review from grosser September 15, 2022 01:19
@grosser grosser merged commit c828e0a into grosser:master Sep 15, 2022
@grosser
Copy link
Owner

grosser commented Sep 15, 2022

need a release or more coming ?

@zdrve
Copy link
Collaborator Author

zdrve commented Sep 15, 2022

In terms of the "smarter diff" feature, that's all that's needed. Smaller releases are better than big anyway :-)

@grosser
Copy link
Owner

grosser commented Sep 15, 2022 via email

@zdrve zdrve deleted the zdrve/smarter-update-diff-simpler branch October 23, 2022 20:17
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