crossplane icon indicating copy to clipboard operation
crossplane copied to clipboard

Bugfix: Wrong filenames after includes in certain scenarios

Open pbitty opened this issue 3 years ago • 4 comments

Proposed changes

This fixes a bug that occurs when an include exists in the middle of a file and Combine=True is set. The directives that appear after the include in the outer file will have the wrong filename on them, subject to the number of directives in the included file.

The failing test in the first commit exposes the bug. This bug was caused by a loop variable mutating the value of a variable by the same name in an outer scope.

Checklist

  • [x] I have read the CONTRIBUTING doc
  • [x] I have added tests that prove my fix is effective or that my feature works
  • [x] I have checked that all unit tests pass after adding my changes
  • [x] I have updated necessary documentation
  • [x] I have rebased my branch onto master
  • [x] I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

pbitty avatar Mar 16 '21 19:03 pbitty

Hello. Is anyone available to give some feedback on this? If I am not following the right process, please let me know.

pbitty avatar Oct 25 '21 13:10 pbitty

Ping! :)

Is anyone around to give some feedback on this PR?

pbitty avatar Dec 03 '21 16:12 pbitty

Hi @pbitty - thanks for your PR. Indeed, this is a correct place to post any fixes around crossplane, so you're doing it right - no worries at all! Unfortunately, the project is a bit stalled nowadays - but we still use it as a part of NGINX Amplify, so any contributions, including bug fixes, are greatly valued. I hope to see some improvements on regular process of checking this and other Amplify-related public repositories for reports and issues; in the meantime, let me try to seek for appropriate resources to review your particular one.

Sorry for not being as responsive as we could, and thank you again for your attention.

defanator avatar Dec 06 '21 08:12 defanator

Hi @defanator, no problem at all, I appreciate your response.

pbitty avatar Dec 06 '21 15:12 pbitty