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: Add recursive directory check for --restrict-file-list #4972

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

Conversation

ibalat
Copy link

@ibalat ibalat commented Oct 1, 2024

This change enhances the file change detection logic in Atlantis to support recursive directory checks for --restrict-file-list parameter. Previously, only direct changes to specified directories were considered for plan and apply. With this update, changes in subdirectories are also detected, ensuring resources that depend on configuration files in nested directories are properly applied.

What:

  • Modified the file change check to recursively detect changes in subdirectories.
  • Ensures that resources in parent directories are applied even when only files in subdirectories are modified.

Why:

In cases where Terraform resources in a directory (e.g., clusters/dev) depend on configuration files from subdirectories (e.g., clusters/dev/configs/dashboards/application), changes to those subdirectories were not triggering the necessary plan and apply commands. This fix resolves that issue, improving the workflow when working with nested configurations.

references

ISSUE #4971

@ibalat ibalat requested review from a team as code owners October 1, 2024 10:23
@ibalat ibalat requested review from GenPage, lukemassa and nitrocode and removed request for a team October 1, 2024 10:23
@github-actions github-actions bot added the go Pull requests that update Go code label Oct 1, 2024
@dosubot dosubot bot added the feature New functionality/enhancement label Oct 1, 2024
@@ -629,8 +629,10 @@ func (p *DefaultProjectCommandBuilder) buildProjectPlanCommand(ctx *command.Cont
foundDir := false

for _, f := range modifiedFiles {
if filepath.Dir(f) == cmd.RepoRelDir {
// Check if the file is in the directory or any subdirectory of the target
if strings.HasPrefix(filepath.Dir(f), cmd.RepoRelDir) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I would hide this behind a flag to make sure people that want to be very strict can still do so.

Copy link
Author

@ibalat ibalat Oct 1, 2024

Choose a reason for hiding this comment

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

so should we bind this logic to another parameter like --restrict-file-list-recursive ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ibalat, yeah, that looks like a good flag to me. Just be aware that I can review this, but I'm not sure I'm the best person to give a final word if this should be accepted.

Copy link
Contributor

Choose a reason for hiding this comment

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

I totally agree with @Fabianoshz. Introducing that flag could help to backwards compatibility 👍

Copy link
Author

Choose a reason for hiding this comment

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

okey, thanks for your comments. I'll update the pr

@X-Guardian
Copy link
Contributor

Thanks for this @ibalat. Can you also look at adding some unit tests for your new functionality.

@X-Guardian X-Guardian added the needs tests Change requires tests label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality/enhancement go Pull requests that update Go code needs tests Change requires tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants