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: Handle glob pattern in watch configuration path #12557

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jhrotko
Copy link
Contributor

@jhrotko jhrotko commented Feb 17, 2025

What I did
This PR enables watch configuration to handle glob patterns in watch configuration path keyword.

Example:

    develop:
      watch:
        - path: api/*.txt
          action: rebuild

In order to watch files under some glob pattern, during watch initialisation we register the anchor folder for the glob pattern (ex.: api). When a file event is received, is going to check if any of the watch rules matches the pattern. In this case, an additional rule pattern was added to watchRule -- globPattern -- in order to fulfil this verification.

Note: Some of these changes require changes made in compose-spec/compose-go#740. For more details please check this PR.

Related issue

This PoC is based on the request

(not mandatory) A picture of a cute animal, if possible in relation to what you did

go.mod Outdated
@@ -198,3 +198,5 @@ require (
sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect
sigs.k8s.io/yaml v1.4.0 // indirect
)

replace github.com/compose-spec/compose-go/v2 v2.4.8 => ../compose-go
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @jhrotko 👋
You need to replace compose-go with a version of your fork 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coucou @glours ! 😉
Done

@ndeloof
Copy link
Contributor

ndeloof commented Feb 20, 2025

LGTM

just as a side not, support for multiple paths in watch definition could also be implemented as some syntaxic sugar in compose-go, i.e. while parsing we translate multiple path values into a set of watch rules, with same action definition, for each declared paths. Somehow comparable to the way we translate port ranges into a set of simpler port declarations.

This would avoid changes to compose and keep logic simpler (especially to later introduce support for match patterns). wdyt?

@jhrotko
Copy link
Contributor Author

jhrotko commented Feb 20, 2025

LGTM

just as a side not, support for multiple paths in watch definition could also be implemented as some syntaxic sugar in compose-go, i.e. while parsing we translate multiple path values into a set of watch rules, with same action definition, for each declared paths. Somehow comparable to the way we translate port ranges into a set of simpler port declarations.

This would avoid changes to compose and keep logic simpler (especially to later introduce support for match patterns). wdyt?

Hey @ndeloof :) Could you confirm that when you say sugar syntax do you mean to represent the path like this? (keeping the string type in compose-spec, etc?)

...
   develop:
      watch:
        - path: api;other;*.txt;web/some/really/super/long/path; ### assuming ; as a delimiter (por example) for each path
          action: rebuild

@ndeloof
Copy link
Contributor

ndeloof commented Feb 20, 2025

Nope, I mean path can be a string or list in yaml as you suggested, but compose-go while loading would actually convert this into a single path string per watch rule, so that you don't need this PR.

i.e.

   develop:
      watch:
        - path: [api, other]
           action: rebuild

would be cannonical-ized into:

   develop:
      watch:
        - path: api
           action: rebuild
        - path: other
           action: rebuild

Actually I see more benefits in pattern support vs multiple paths, which sounds a corder case to me

@jhrotko
Copy link
Contributor Author

jhrotko commented Feb 20, 2025

Nope, I mean path can be a string or list in yaml as you suggested, but compose-go while loading would actually convert this into a single path string per watch rule, so that you don't need this PR.

i.e.

   develop:
      watch:
        - path: [api, other]
           action: rebuild

would be cannonical-ized into:

   develop:
      watch:
        - path: api
           action: rebuild
        - path: other
           action: rebuild

Actually I see more benefits in pattern support vs multiple paths, which sounds a corder case to me

I see, I will make this changes. The pattern matching can translate into multiple files though so these features are actually related. And can be annoying to define multiple watch rules for the same action (rebuild and restart)

@ndeloof
Copy link
Contributor

ndeloof commented Feb 21, 2025

The pattern matching can translate into multiple files though so these features are actually related

I don't get this. A basic path matches multiple files (anything under folder matches). A pattern is just more restrictive. This is unrelated to multiple values in path (which we can support, but this is another feature then)

My initial thoughts to implement this feature was to introduce an includes attribute to define pattern filtering for files under path, but it makes sense for a simpler approach to make path a pattern. The fixed part of the pattern would then declare the source path, and the match expression the filter
For illustration:

path: src/*.txt # watch src/ but only consider .txt files
target: /app

File src/foo.txt would be copied into container as app/foo.txt
This would be equivalent to a negative ignore:

path: src/
ignore: !*.txt # AFAIK this isn't supported
target: /app

.. but with a more comprehensible syntax :P

@jhrotko
Copy link
Contributor Author

jhrotko commented Feb 21, 2025

so imagine the case where the target is a file

  path: app/*.txt
  target: app/some.txt
  action: sync

in this case, we need to be careful and through some error. During validation, when target is a file, compose-go needs to assert the relation 1 to 1 is true or else it will have a weird behaviour in this case. That is why I was assuming path could be treated as a []string to be easier to validate this case.

The presented example I would assume it should throw an error or show a warning at least as multiple files can overwrite one file in the container

@ndeloof
Copy link
Contributor

ndeloof commented Feb 21, 2025

target can't be a file, MUST be a folder. If you want to sync with file app/some.txt inside container, source MUST be some.txt. What you describe is way more complex use-case but just support for pattern

Remember sync is basically equivalent to a bind mount (without the constraints of a bind mount)

@jhrotko jhrotko force-pushed the feature-wild-star-watch-path branch from 3e40780 to 40bcdcc Compare February 25, 2025 15:52
@jhrotko jhrotko marked this pull request as ready for review February 25, 2025 15:54
@jhrotko jhrotko requested a review from a team as a code owner February 25, 2025 15:54
@jhrotko jhrotko requested review from ndeloof and glours February 25, 2025 15:54
@jhrotko jhrotko changed the title Process multiple paths for watch config feat: Handle glob pattern in watch configuration path Feb 25, 2025
@jhrotko jhrotko force-pushed the feature-wild-star-watch-path branch from 40bcdcc to 4396577 Compare February 25, 2025 16:02
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