fix: validate whole plan, rather than topmost two layers #363
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
flotter
approved these changes
Feb 20, 2024
|
@tonyandrewmeyer b.t.w this is a real coincidence that you are cleaning this up a bit, because I am on a kind of similar mission for slightly different reasons, and I've done in my PoC exactly what you conceptionally doing here. I've also split it up between Layer and Plan validation, but you went a bit further, actually cleaning up what is inside each category, which is super nice and brave :) Looking forward getting this merged. |
benhoyt
requested changes
Feb 22, 2024
benhoyt
reviewed
Feb 22, 2024
benhoyt
requested changes
Feb 23, 2024
benhoyt
pushed a commit
to benhoyt/pebble
that referenced
this pull request
Mar 5, 2024
This PR moves layer/plan validation into dedicated functions, with `Layer.Validate` covering validation that is specific to a layer (such as an invalid value) and `Plan.Validate` covering validation that covers the plan as a whole (such as cycles or missing values). Plan validation includes layer validation, and is done in the service manager's `updatePlanLayers` and in `plan.CombineLayers`. We also use the check period as the upper bound for the timeout in checks, which was happening before, but less explicitly. No additional validation is done - all of the validation code is moved rather than changed. Fixes canonical#349
jujubot
pushed a commit
to juju/juju
that referenced
this pull request
Mar 6, 2024
Validation fix PR: canonical/pebble#363 Pebble release: https://github.com/canonical/pebble/releases/tag/v1.7.3
jujubot
added a commit
to juju/juju
that referenced
this pull request
Mar 6, 2024
#17011 This is to pull in this Pebble fix to the Juju 3.4 branch: canonical/pebble#363. It fixes an issue a couple of people have had with adding and merging layers (described in canonical/pebble#349).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves layer/plan validation into dedicated functions, with
Layer.Validatecovering validation that is specific to a layer (such as an invalid value) andPlan.Validatecovering validation that covers the plan as a whole (such as cycles or missing values).Plan validation includes layer validation, and is done in the service manager's
updatePlanLayersand inplan.CombineLayers.We also use the check period as the upper bound for the timeout in checks, which was happening before, but less explicitly.
No additional validation is done - all of the validation code is moved rather than changed.
Fixes #349