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

feat(plan): add plan sub-directory support #509

Merged
merged 15 commits into from
Oct 23, 2024

Conversation

flotter
Copy link
Contributor

@flotter flotter commented Oct 13, 2024

Pebble currently only supports loading configuration layer files from the root of $PEBBLE/layers.

Add support for adding sub-directory support at the root level (2 levels in total only). Spec KO071.

This scheme has an impact on the meaning of label and order. See the following examples to understand the new mapping:

  • $PEBBLE/layers/001-foo.yaml
    Order: 001-000 => 1000
    Label: foo

  • $PEBBLE/layers/005-bar.d/010-abc.yaml
    Order: 005-010 => 5010
    Label: bar/abc

  • $PEBBLE/layers/005-bar.d/012-def.yaml
    Order: 005-012 => 5012
    Label: bar/def

  • $PEBBLE/layers/006-baz.yaml
    Order: 006-000 => 6000
    Label: baz

Directory support allows other uses of Pebble using read-only file systems more flexibility by now giving the option to bind-mount a sub-directory to a writable disk location, for example.

@flotter
Copy link
Contributor Author

flotter commented Oct 13, 2024

@benhoyt Hi Ben, would it be possible for you to give this a initial scan for me please, when you have some time? I want to see if there are early course corrections needed.

Outstanding tasks:

  1. Unit Tests outside of plan still needs fixing
  2. Unit Tests missing for new functionality in Plan (current tests pass).
  3. Some logic required in planstate (Append / Combine) to deal with requests coming in over the API.

Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

I've had a cursory look. Overall structure looks reasonable.

My main comment is the regexes are getting pretty hard to understand (and get right!). I wonder if we can have a simple regex that just matches the "001-label" part, and do the other parts with strings.Split and filepath.Ext / filepath.Base.

The other thing I wonder if you should consider is combining the directory traversal with the parsing -- because when you're traversing the directory you've already split on / so you have more information. I'm not sure, but it seems like that would eliminate a bit of the parsing by design.

client/plan.go Outdated Show resolved Hide resolved
@flotter
Copy link
Contributor Author

flotter commented Oct 14, 2024

I've had a cursory look. Overall structure looks reasonable.

My main comment is the regexes are getting pretty hard to understand (and get right!). I wonder if we can have a simple regex that just matches the "001-label" part, and do the other parts with strings.Split and filepath.Ext / filepath.Base.

The other thing I wonder if you should consider is combining the directory traversal with the parsing -- because when you're traversing the directory you've already split on / so you have more information. I'm not sure, but it seems like that would eliminate a bit of the parsing by design.

@benhoyt thank you for finding the time to have a look. Exactly the guidance I needed thank you. Your proposal makes a lot of sense and I made the following changes in line with what you are proposing:

  1. I removed all the complex regex expressions and kept only one simple one (similar to the original one).
  2. I moved the loading into the tree traversal code, which meant I have access to the already split orders/labels.

This allowed me to remove and simplify lots of complex code.

TODO: I will proceed with completing the outstanding tasks here, and report back for a more thorough review.

@flotter flotter changed the title feat(plan): add support layer files in a sub-dir feat(plan): add plan sub-directory support Oct 14, 2024
@flotter flotter requested a review from hpidcock October 14, 2024 22:13
@flotter flotter marked this pull request as ready for review October 14, 2024 22:15
@flotter flotter requested a review from anpep October 15, 2024 07:45
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Looks very good, thanks! Some comments, but almost all minor / stylistic.

internals/plan/plan.go Outdated Show resolved Hide resolved
internals/plan/plan.go Outdated Show resolved Hide resolved
internals/plan/plan.go Outdated Show resolved Hide resolved
internals/plan/plan.go Outdated Show resolved Hide resolved
internals/plan/plan.go Outdated Show resolved Hide resolved
internals/overlord/planstate/manager_test.go Show resolved Hide resolved
internals/overlord/planstate/manager_test.go Outdated Show resolved Hide resolved
internals/daemon/api_plan.go Show resolved Hide resolved
internals/overlord/planstate/manager.go Outdated Show resolved Hide resolved
internals/overlord/planstate/manager.go Outdated Show resolved Hide resolved
client/plan.go Outdated Show resolved Hide resolved
@benhoyt
Copy link
Contributor

benhoyt commented Oct 21, 2024

To record our discussion about API label validation: let's revert that part for now. It's going to be problematic for backwards compatibility, as unfortunately real Juju charms use both '_' and ' ' (space) character in label names right now. Later we can add strict validation for the "persist" case, as that will be a new feature. (mongodb-k8s-operator example of underscore, loki-k8s-operator example of space -- in test charm as part of integration tests).

Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Thanks!

@flotter flotter merged commit b783466 into canonical:master Oct 23, 2024
18 checks passed
@benhoyt benhoyt deleted the plan-directory-support branch October 23, 2024 13:08
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