-
Notifications
You must be signed in to change notification settings - Fork 55
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
Conversation
ab26055
to
d11bf27
Compare
d11bf27
to
527de7d
Compare
@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:
|
There was a problem hiding this 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.
@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:
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. |
There was a problem hiding this 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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
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
andorder
. 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.