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

Add --exclude-processing option #1963

Conversation

lliissoonngg
Copy link
Contributor

@lliissoonngg lliissoonngg commented Jan 12, 2024

Fixes #1911

This PR added a CLI option --exclude-processing and an equivelent config entry excludeProcessing, if it is set, the matching files will go with a different code path copyFileToOutDir.

To make the code reusable, the codes about file IO are offloaded fromfileToTemplate into readInFile, getOutfileHandler, and are called by copyFileToOutDir as well.

How to test:

Have a jpeg file in in directory.

case 1: (test cli argument --exclude-processing)

run:
./gomplate --exclude-processing '*.jpg' --input-dir=in --output-dir=out

the *.jpg exists in out directory

case 2: (test config file entry excludeProcessing)

have a .gomplate.yaml file in the directory:
`
inputDir: in/
outputDir: out/
excludeProcessing:

  • '*.jpg'
    `

run:
./gomplate --input-dir=in --output-dir=out

the *.jpg exists in out directory

@lliissoonngg lliissoonngg marked this pull request as draft January 12, 2024 22:56
@hairyhenderson
Copy link
Owner

Hi @lliissoonngg, thanks for working on this.

Before I review this fully, a couple things:

  1. some integration tests need to be written for this (see internal/tests/integration/)
  2. I'm not sure about the name of this flag - "exclude-processing" isn't very clear - perhaps you have an idea on a better name? Or maybe @assayire from the original issue?

@@ -139,7 +139,8 @@ func InitFlags(command *cobra.Command) {
command.Flags().StringP("in", "i", "", "Template `string` to process (alternative to --file and --input-dir)")
command.Flags().String("input-dir", "", "`directory` which is examined recursively for templates (alternative to --file and --in)")

command.Flags().StringSlice("exclude", []string{}, "glob of files to not parse")
command.Flags().StringSlice("exclude", []string{}, "glob of files to ignore")
Copy link
Owner

Choose a reason for hiding this comment

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

I think the previous text was fine - "ignore" is somewhat ambiguous in this context

@hairyhenderson hairyhenderson changed the title [feature][Issue 1911] add --exclude-processing option Add --exclude-processing option Jan 13, 2024
template.go Outdated Show resolved Hide resolved
@lliissoonngg
Copy link
Contributor Author

@assayire gently bump on this, could you take a look of the parameter naming in this PR?

@assayire
Copy link
Contributor

./gomplate --exclude-processing '*.jpg' --input-dir=in --output-dir=out

I am fine with the name.

Quick question: If I have exclude say jpg and png, would I do --exclude-processing '*.jpg,*.png'?

@lliissoonngg
Copy link
Contributor Author

./gomplate --exclude-processing '*.jpg' --input-dir=in --output-dir=out

I am fine with the name.

Quick question: If I have exclude say jpg and png, would I do --exclude-processing '*.jpg,*.png'?

No, --excludes has higher priority, in this case jpg and png will not be copied. Let me know if this is good enough. I will update the doc after we all agree upon a working logic.

@assayire
Copy link
Contributor

./gomplate --exclude-processing '*.jpg' --input-dir=in --output-dir=out

I am fine with the name.
Quick question: If I have exclude say jpg and png, would I do --exclude-processing '*.jpg,*.png'?

No, --excludes has higher priority, in this case jpg and png will not be copied. Let me know if this is good enough. I will update the doc after we all agree upon a working logic.

Oops .. i think i typed it wrong. I meant to ask that if i have exclude multiple files from processing, would this work - --exclude-processing '*.jpg,*.png,*.gif'?

@lliissoonngg lliissoonngg marked this pull request as ready for review January 18, 2024 15:52
@lliissoonngg lliissoonngg marked this pull request as draft January 18, 2024 16:06
@lliissoonngg
Copy link
Contributor Author

./gomplate --exclude-processing '*.jpg' --input-dir=in --output-dir=out

I am fine with the name.
Quick question: If I have exclude say jpg and png, would I do --exclude-processing '*.jpg,*.png'?

No, --excludes has higher priority, in this case jpg and png will not be copied. Let me know if this is good enough. I will update the doc after we all agree upon a working logic.

Oops .. i think i typed it wrong. I meant to ask that if i have exclude multiple files from processing, would this work - --exclude-processing '*.jpg,*.png,*.gif'?

Yes, multiple patterns works (exactly the same as --excludes).

@lliissoonngg lliissoonngg marked this pull request as ready for review January 18, 2024 16:08
@hairyhenderson
Copy link
Owner

would this work - --exclude-processing '*.jpg,*.png,*.gif'?

If it works like --exclude then I think it would not be a comma-separated string, but instead multiple options, so:

--exclude-processing '*.jpg' --exclude-processing '*.png' --exclude-processing '*.gif'

For something so verbose as this it would make more sense to use a config file, which would look like:

excludeProcessing
  - '*.jpg'
  - '*.png'
  - '*.gif'

Hope that makes sense @assayire

@hairyhenderson
Copy link
Owner

@lliissoonngg thanks for the updates. Can you please rebase? I've just merged #1336 which makes a large number of changes that will affect this.

I'll make another review pass after that.

@tewfik-ghariani
Copy link

Would it be also possible to exclude the processing of certain files even if "input" and "output" are not specified ( regardless of the copying operation)?
For example

$ ls -1 helm-values/
template.yml.tmpl
values.yml

$ gomplate -f helm-values/ --exclude-processing  '*.yml'

@hairyhenderson
Copy link
Owner

@tewfik-ghariani that command doesn't really make sense - -f must refers to a file, not a directory. This option would only make sense when using --input-dir.

@tewfik-ghariani
Copy link

Ah right, @hairyhenderson thanks for the clarification ^^

@assayire
Copy link
Contributor

would this work - --exclude-processing '*.jpg,*.png,*.gif'?

If it works like --exclude then I think it would not be a comma-separated string, but instead multiple options, so:

--exclude-processing '*.jpg' --exclude-processing '*.png' --exclude-processing '*.gif'

For something so verbose as this it would make more sense to use a config file, which would look like:

excludeProcessing
  - '*.jpg'
  - '*.png'
  - '*.gif'

Hope that makes sense @assayire

@hairyhenderson How would you specify it on command line to --exclude-processing? I mean the multiple files as you have specified in yaml.

@hairyhenderson
Copy link
Owner

@assayire like I said:

If it works like --exclude then I think it would not be a comma-separated string, but instead multiple options, so:

--exclude-processing '*.jpg' --exclude-processing '*.png' --exclude-processing '*.gif'

@lliissoonngg lliissoonngg force-pushed the song/issue-1911-exclude-processing branch from 6ef96f2 to 19caa0d Compare January 23, 2024 05:21
@assayire
Copy link
Contributor

LGTM

@lliissoonngg lliissoonngg force-pushed the song/issue-1911-exclude-processing branch from d8d4b2c to e8a4261 Compare January 27, 2024 03:56
Copy link
Owner

@hairyhenderson hairyhenderson left a comment

Choose a reason for hiding this comment

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

Thanks again for this contribution @lliissoonngg! I'm going to rebase and merge this now, and will work (later) on replacing the xignore module.

@hairyhenderson hairyhenderson force-pushed the song/issue-1911-exclude-processing branch from 35e489e to 19b2ef4 Compare February 6, 2024 01:27
@hairyhenderson hairyhenderson enabled auto-merge (squash) February 6, 2024 01:27
@hairyhenderson hairyhenderson merged commit 4d24b9d into hairyhenderson:main Feb 6, 2024
13 checks passed
@assayire
Copy link
Contributor

Is this update released?

@mgbowman
Copy link

@assayire, it doesn't look like it. This was merged on Feb 6 and the v3.11.7 release was published on Jan 23, which is a pity because I could really use this for my current project. 😞

@hairyhenderson any chance we can get a v3.x release with this option or do we need to wait for v4.x?

@hairyhenderson
Copy link
Owner

This is a new feature and as such is going in the next release, which will be v4.

I could cut a v4.0.0-pre-3 pre-release if that helps.

(also in general please don't pose questions on closed issues, they're very easy to miss!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Exclude from processing but still copy file to target folder
5 participants