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

Added exclude_from_copy to config #3543

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

Conversation

KabaevRoman
Copy link

@KabaevRoman KabaevRoman commented Nov 10, 2024

Description

Fixes #916.

Added exclude_from_copy to config, to exclude some paths via globs

TODOs

Read the Gruntwork contribution guidelines.

  • Update the docs.
  • Run the relevant tests successfully, including pre-commit checks.
  • Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.
  • Include release notes. If this PR is backward incompatible, include a migration guide.

Release Notes (draft)

Added / Removed / Updated [X].

Migration Guide

@KabaevRoman
Copy link
Author

Don't know if exclude_in_copy syntactically correct, if I should rename exclude_from_copy to exclude_in_copy, or made mistakes in formatting/code let me know!

@KabaevRoman
Copy link
Author

KabaevRoman commented Nov 10, 2024

config tests > https://gist.github.com/KabaevRoman/17cbee1e7cacb6e154d0ae18945c898f
util tests > https://gist.github.com/KabaevRoman/f7f4bc1aa456d9f0326547db8fa4b4bf
Not really sure how to run integration tests, I don't have an aws account available, if it's possible would be nice to kick of pipeline in github

Copy link
Collaborator

@yhakbar yhakbar left a comment

Choose a reason for hiding this comment

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

Hey @KabaevRoman,

Sorry we haven't gotten around to reviewing this until now. I'll look to kick off CI later today.

In the meantime, do you mind adding documentation and testing specifically around the scenario where files are listed as both included and excluded? I would assume that the safest thing to do would be to exclude them in that circumstance.

You should be able to run most of the tests in this repository without an AWS account, etc. We've done some work fairly recently to move all tests that require external integrations to require build tags, so they shouldn't run by default.

You might want to be targeted with your tests by doing something like this anyways, though:

go test -run 'NameOfTest'

We have a goal of making it so that contributors can easily run tests locally, so any help you could provide to make that a better experience would be much appreciated.

@yhakbar
Copy link
Collaborator

yhakbar commented Jan 7, 2025

Closing this PR for now, as it has gone stale. We can re-open it or look at a new version of the PR if you're willing to revisit it, @KabaevRoman .

@yhakbar yhakbar closed this Jan 7, 2025
@KabaevRoman
Copy link
Author

KabaevRoman commented Jan 7, 2025

@yhakbar Hi there, missed previous message,
I've updated the docs here https://github.com/gruntwork-io/terragrunt/pull/3543/files#diff-eac47411aff1ec957598f585714c7199a1c1c31a710ef472868bae5abef38cf9R111, do i need to update them somewhere else?
As for tests I've added tests for behaviour that you described here https://github.com/gruntwork-io/terragrunt/pull/3543/files#diff-4146922e3bcd164a817418c48bfd4ee62b79863281d449038daab9109c81cfbdR431, i think you can re-open pr and i will resolve existing conflicts.
Give me a hint what tests should i run for this pr, currently my tests are ok, but I'm a bit worried that I could break something,
but running all tests locally as described in docs seems to be not working properly, cause I get errors with some .hcl test files paths or something like that.

@yhakbar
Copy link
Collaborator

yhakbar commented Jan 7, 2025

No problem, @KabaevRoman ! I'll re-open the PR as you're up for picking it up.

The documentation I'm looking to see here is very explicitly telling users what happens when both include_in_copy and exclude_from_copy are defined.

I see that you added this:

  When `include_in_copy` is provided, you can still provide `exclude_from_copy` to skip provided glob, or nested glob from `include_in_copy` 

But as a user reading these docs, I would rather see something like this:

Note that using `include_in_copy` and `exclude_from_copy` are not mutually exclusive. 

If a file matches a pattern in both `include_in_copy` and `exclude_from_copy`, it will not be included. If you would like to ensure that the file _is_ included, make sure the patterns you use for `include_in_copy` do not match the patterns in `exclude_from_copy`.

You don't need to run all the tests. Only run the tests that you think are relevant locally. Once you've resolved conflicts, I can re-run tests for you in CI. Note that quite a lot of work is currently underway for the code paths that you're touching here #3723, so we might need to wait a little bit to merge this so that we can keep development unblocked for Terragrunt 1.0 work.

@yhakbar yhakbar reopened this Jan 7, 2025
@KabaevRoman KabaevRoman force-pushed the feature/exclude-from-copy branch from 3ceb2d0 to 500263f Compare January 7, 2025 21:24
@KabaevRoman KabaevRoman force-pushed the feature/exclude-from-copy branch from 500263f to 796e3b7 Compare January 7, 2025 21:32
@KabaevRoman
Copy link
Author

I've updated pr and changed readme as you advised, tests seems to be passing

@yhakbar
Copy link
Collaborator

yhakbar commented Jan 7, 2025

Thanks @KabaevRoman . I've kicked off CI. I'll coordinate with the team to see if we can merge this in earlier at the risk of increased conflicts, or if we should hold off for a bit. Either way, thanks for your patience and working with us to get this done!

@KabaevRoman KabaevRoman force-pushed the feature/exclude-from-copy branch from 297032f to 545a631 Compare January 8, 2025 12:06
@KabaevRoman
Copy link
Author

KabaevRoman commented Jan 8, 2025

Some tests failed in pipeline, i was able to discover linting errors and how to run linters from makefile and fixed them, but got no luck with running integration tests properly even unit_test from config.yml. Also i have no access to pipeline logs to get more details for failing tests. And have no idea what is this shortcut you are using run-go-tests

@yhakbar
Copy link
Collaborator

yhakbar commented Jan 8, 2025

Hey @KabaevRoman ,

And have no idea what is this shortcut you are using run-go-test

It's an internal tool we're using that basically just runs go test. We've discussed internally this frustration you're experiencing that the community can't reproduce our CI is really problematic, and we're going to remove it in the future.

got no luck with running integration tests properly even unit_test from config.yml

That's definitely not OK! We know we can do a better job of making our tests reproducible by the community, so if you could open a bug on the unit tests that you can't run locally, we'd like to fix them.

Ideally, a user would be able to run go test ./... after cloning the repo without issue, as one would expect. We've allowed some integration testing to get muddled into our unit tests that prevents this, and we want to fix it.

This test is failing right now:

=== NAME  TestReadTerragruntConfigFull
    integration_test.go:2393: 
        	Error Trace:	/home/circleci/project/test/integration_test.go:2393
        	Error:      	Not equal: 
        	            	expected: map[string]interface {}{"after_hook":map[string]interface {}{"after_hook_1":map[string]interface {}{"commands":[]interface {}{"apply", "plan"}, "execute":[]interface {}{"touch", "after.out"}, "name":"after_hook_1", "run_on_error":true, "suppress_stdout":interface {}(nil), "working_dir":interface {}(nil)}}, "before_hook":map[string]interface {}{"before_hook_1":map[string]interface {}{"commands":[]interface {}{"apply", "plan"}, "execute":[]interface {}{"touch", "before.out"}, "name":"before_hook_1", "run_on_error":true, "suppress_stdout":interface {}(nil), "working_dir":interface {}(nil)}}, "copy_terraform_lock_file":true, "error_hook":map[string]interface {}{}, "exclude_from_copy":[]interface {}{"excluded_time_machine.*"}, "extra_arguments":map[string]interface {}{"var-files":map[string]interface {}{"arguments":interface {}(nil), "commands":[]interface {}{"apply", "plan"}, "env_vars":map[string]interface {}{"TF_VAR_custom_var":"I'm set in extra_arguments env_vars"}, "name":"var-files", "optional_var_files":[]interface {}{"optional.tfvars"}, "required_var_files":[]interface {}{"extra.tfvars"}}}, "include_in_copy":[]interface {}{"time_machine.*"}, "source":"./delorean"}
        	            	actual  : map[string]interface {}{"after_hook":map[string]interface {}{"after_hook_1":map[string]interface {}{"commands":[]interface {}{"apply", "plan"}, "execute":[]interface {}{"touch", "after.out"}, "name":"after_hook_1", "run_on_error":true, "suppress_stdout":interface {}(nil), "working_dir":interface {}(nil)}}, "before_hook":map[string]interface {}{"before_hook_1":map[string]interface {}{"commands":[]interface {}{"apply", "plan"}, "execute":[]interface {}{"touch", "before.out"}, "name":"before_hook_1", "run_on_error":true, "suppress_stdout":interface {}(nil), "working_dir":interface {}(nil)}}, "copy_terraform_lock_file":true, "error_hook":map[string]interface {}{}, "exclude_from_copy":interface {}(nil), "extra_arguments":map[string]interface {}{"var-files":map[string]interface {}{"arguments":interface {}(nil), "commands":[]interface {}{"apply", "plan"}, "env_vars":map[string]interface {}{"TF_VAR_custom_var":"I'm set in extra_arguments env_vars"}, "name":"var-files", "optional_var_files":[]interface {}{"optional.tfvars"}, "required_var_files":[]interface {}{"extra.tfvars"}}}, "include_in_copy":[]interface {}{"time_machine.*"}, "source":"./delorean"}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -36,5 +36,3 @@
        	            	  },
        	            	- (string) (len=17) "exclude_from_copy": ([]interface {}) (len=1) {
        	            	-  (string) (len=23) "excluded_time_machine.*"
        	            	- },
        	            	+ (string) (len=17) "exclude_from_copy": (interface {}) <nil>,
        	            	  (string) (len=15) "extra_arguments": (map[string]interface {}) (len=1) {
        	Test:       	TestReadTerragruntConfigFull
--- FAIL: TestReadTerragruntConfigFull (3.00s)

@KabaevRoman
Copy link
Author

Fixed test that you mentioned, it passes now. Maybe i will describe later all issues i encountered, on running tests linters etc. It seems like docs are a bit outdated https://terragrunt.gruntwork.io/docs/community/contributing/#developing-terragrunt, because they don't mention linters at all, but again i better describe all issues i encountered in separate issue or something.

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.

Exclude copying folder from working directory to cache folder
2 participants