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 option to ignore files/dirs #424

Closed
wants to merge 2 commits into from
Closed

Conversation

pkit
Copy link

@pkit pkit commented Jan 15, 2023

adds a new option to all workflow builders: ignore specific files and directories

ex:

{
  options: { "ignore": [".build", ".tmp*"] }
}

fixes: #382
fixes: #185

And other numerous issues asking for a simple ignore feature

Added a test for python because I don't care for anything else right now.
But the feature is fully backwards compatible anyway.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

adds a new option into all workflow builders
to ignore specific files and directories

ex:

{
  options: { "ignore": [".build", ".tmp*"] }
}
@pkit pkit requested a review from a team as a code owner January 15, 2023 02:53
@pkit pkit requested review from sriram-mv and mildaniel January 15, 2023 02:53
@carpnick
Copy link

Does it solve #185? - The fact Excluded files is hard coded is the problem. As I see this, isnt this still using the hard coded EXCLUDED_FILES var which means it still wont fix the use case as far as I am aware.

@pkit
Copy link
Author

pkit commented Jan 18, 2023

Hmm, I wanted it to be backwards compatible. But it's not that hard to improve. For example, I can add exclude option, if set to [] it will remove all the hardcoded excludes.

@carpnick
Copy link

carpnick commented Jan 18, 2023

Hmm, I wanted it to be backwards compatible. But it's not that hard to improve. For example, I can add exclude option, if set to [] it will remove all the hardcoded excludes.

I think the easier route would be allow exclude to be set. If unset, set to default EXCLUDED_FILES, then this allows the SAM repo to expose a parameter to send in your own ignore patterns.

Really there are 2 use cases here:

  • What is excluded from final package
  • What is ignored when creating the initial copy of the source.

The question - will end users see them being one in the same? Or are they separate use cases?

@pkit
Copy link
Author

pkit commented Jan 18, 2023

I'm not sure I want to force users to set the existing hundred excludes for python, just because they need to add only one.

@carpnick
Copy link

I'm not sure I want to force users to set the existing hundred excludes for python, just because they need to add only one.

Good point. In #185 use case I need to remove some :). So the only way I know how to support both is you override or dont.

@torresxb1
Copy link
Contributor

torresxb1 commented Jan 18, 2023

Thanks for bringing this to our attention and thanks for the discussion! Since it seems like we have some use cases where we want to ignore more things than EXCLUDED_FILES and some use cases where we want to ignore less things (like @carpnick 's case), then I do think the best solution might be some way of exposing control of this EXCLUDED_FILES. Not sure if the best way to do would be an option, an environment variable, some config file, or something else.

But I do see @pkit 's concern:

I'm not sure I want to force users to set the existing hundred excludes for python, just because they need to add only one.

Maybe the best way would be allowing three possibilities of controlling EXCLUDED_FILES:

  • setting it (to manually specify/control this list)
  • adding to it (to specify more files, on top of the default ones, to ignore)
  • removing from it (to specify which files not to ignore from the default list)

Anyway, will discuss with the team, and put this PR in draft in the meantime. Let us know if you have more thoughts!

@torresxb1 torresxb1 marked this pull request as draft January 18, 2023 20:31
@torresxb1 torresxb1 removed the stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. label Jan 18, 2023
@pkit
Copy link
Author

pkit commented Jan 19, 2023

First I wanted to create a .gitignore-like file that can be fed into the json-rpc options.
This way we should solve all the problems even if we merge it with the existing EXCLUDES (as it has ! for disabling paths)
But it looks like there is no good first-class support for gitignore in python, all packages I've checked lack some features or just don't even work. Writing a real gitignore support wasn't my goal, but maybe I'll dedicate some more time to it.

@sriram-mv
Copy link
Contributor

Related to #430 atleast with respect to having a common functionality on includes/excludes: #430

@mildaniel
Copy link
Contributor

Closing until we figure out the correct path forward. Consolidating include/exclude functionality design and discussion into a single issue #516.

@mildaniel mildaniel closed this Jun 12, 2023
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.

Bug: recursively copies artifact dir when it's in source dir Python: Extend and/or override EXCLUDED_FILES
6 participants