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(file): Allows to use file pattern in the file/files key #1738

Merged
merged 16 commits into from
Nov 10, 2023

Conversation

olblak
Copy link
Member

@olblak olblak commented Nov 4, 2023

Fix #1453

This pullrequest allows to use common file path pattern with the plugin "file"

name: Test file resource without scm 

sources:
  adoptersWithFilePattern:
    name: "Get content from each file(s) matching pattern ADOPTE??.md"
    kind: file
    spec:
      file: ADOPTE??.md
      searchpattern: true

Test

To test this pull request, you can run the following commands:

cd pkg/plugins/utils/
go test
go build -o bin/updatecli . 
./bin/updatecli diff --config e2e/updatecli.d/success.d/file/noscm.yaml --debug

Additional Information

Tradeoff

Instead of reading a specific file, the change introduce in this pullrequest will try to locate ALL files matching pattern which obviously has performance consequence...
I could think of two options to mitigate this:

  1. Using a flag to enable/disable this behavior such as
sources:
  adoptersWithFilePattern:
    name: "Get content from each file(s) matching pattern ADOPTE??.md"
    kind: file
    spec:
      file: ADOPTE??.md
      filepattern: true

But I find this approach a bit cumbersome

  1. Using a different key such as

sources:
  adoptersWithFilePattern:
    name: "Get content from each file(s) matching pattern ADOPTE??.md"
    kind: file
    spec:
      filepattern: ADOPTE??.md

But this would imply more code to write and maintain as I'll need to handle the different combination of file,files,filepath, filespath

I don't think the performance impact is that critical but I am open for suggestion

edit
I decided to implement the flag approach using searchpattern instead of filepattern

Potential improvement

Implement similar pullrequest for

  • xml
  • json
  • toml
  • yaml
  • csv

Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
@olblak olblak added enhancement New feature or request resource-file Resource of kind File labels Nov 4, 2023
Signed-off-by: Olblak <me@olblak.com>
@olblak olblak marked this pull request as draft November 4, 2023 14:00
@olblak
Copy link
Member Author

olblak commented Nov 4, 2023

While the idea remain the same, the broken e2e highlight that the current implementation is wrong. It doesn't handle scm

@olblak olblak changed the title chore: Plugin "file" allows to use file pattern in the file/files key feat(file): Allows to use file pattern in the file/files key Nov 4, 2023
@olblak olblak marked this pull request as ready for review November 9, 2023 19:30
@olblak
Copy link
Member Author

olblak commented Nov 9, 2023

I updated the e2e tests to cover the additional scenarios

dduportal
dduportal previously approved these changes Nov 10, 2023
Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

  • First of all, the use case looks really legit and it is a good idea!
  • Code is LGTM
  • No regression with the binary I've tested from this PR (tried with jenkins-infra/jenkins.io updatecli manifests)
  • The "opt in" behavior with addition field is really good
  • Only missing element is: what "kind" of pattern. (Said differently: "as a user where can I find example of the pattern matching?)

=> Approving, but don't forget to add a documentation update even after merging/releasing the feature ;)

@olblak olblak merged commit 278c3f7 into updatecli:main Nov 10, 2023
6 checks passed
@olblak olblak deleted the issue/1453 branch November 10, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request resource-file Resource of kind File
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Allow to use file pattern in the file/files key
2 participants