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

[159] Fix issue with argument parsing logic for tf apply #274

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

borland667
Copy link
Contributor

@borland667 borland667 commented Jun 19, 2024

What?

  • Implemented enhanced parsing logic in handle_apply_arguments_parsing to handle Terraform apply command arguments.
  • Added support for various argument formats including -var, -var-file, -target, -lock, -parallelism, -compact-warnings, -input, -json, -auto-approve, -no-color, and combinations thereof.

Simplified solution:

  • A different solution to the original ended up being implemented, simpler and more maintainable.
  • The idea is to disturb the Terraform arguments the user provides as little as possible previous to passing them to the application, to provide with as a familiar behavior as possible.
  • For Leverage Reference Architecture layers to work as intended, leverage cli needs to pass to Terraform the references to specific variable files via the -var-file arguments. These arguments collide with the presence of a plan file argument, so we need to implement logic to detect whether the user provided such argument or not, and then decide whether we need to add the variable file references to the rest of the arguments or not.

Why?

  • Enhance robustness and flexibility in handling Terraform apply command arguments.
  • Ensure accurate parsing and inclusion of specified arguments while discarding irrelevant or malformed inputs.
  • Improve compatibility with diverse use cases and user input scenarios.
  • Address user feedback regarding parsing inconsistencies and edge cases.

References

  • Use closes #159, if this PR closes a GitHub issue #159

Before release

Review the checklist here

@coveralls
Copy link
Collaborator

coveralls commented Jun 19, 2024

Pull Request Test Coverage Report for Build 9584808751

Details

  • 44 of 63 (69.84%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 69.182%

Changes Missing Coverage Covered Lines Changed/Added Lines %
leverage/logger.py 4 5 80.0%
leverage/modules/terraform.py 36 54 66.67%
Totals Coverage Status
Change from base Build 8847843590: 0.2%
Covered Lines: 2042
Relevant Lines: 2846

💛 - Coveralls

@coveralls
Copy link
Collaborator

coveralls commented Aug 21, 2024

Pull Request Test Coverage Report for Build 10565722330

Details

  • 20 of 24 (83.33%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 69.151%

Changes Missing Coverage Covered Lines Changed/Added Lines %
leverage/logger.py 4 5 80.0%
leverage/modules/terraform.py 12 15 80.0%
Totals Coverage Status
Change from base Build 8847843590: 0.1%
Covered Lines: 2011
Relevant Lines: 2800

💛 - Coveralls

@angelofenoglio
Copy link
Contributor

After consulting with @borland667 I've pushed a simplification to the originally proposed solution.

The only thing we really need to worry is to find out if we are receiving a plan file as a last argument for the apply command. And in turn, prepend or not our terraform default parameters, which basically are the references to the correct .tfvar files.
Instead of parsing the entirety of the parameters, looking at the last two or so received arguments we can deduct if the last one is a positional argument or not (the plan file is the only positional parameter supported by plan). Any other errors/incompatibilities that the user may have introduced in the arguments we leave to terraform to decide.

# Handles '-var' and non '-' starting arguments
new_args.append(arg)
logger.debug(f"Appending argument (non '-' or '-var'): {arg}")
def there_is_a_plan_file(args: Sequence[str]) -> bool:
Copy link
Contributor Author

@borland667 borland667 Aug 22, 2024

Choose a reason for hiding this comment

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

@angelofenoglio I'd update the name to has_plan_file_in_args, I think that is more clear. We're good to merge anyway I reckon!

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe contain_a_plan_file would be better, the args would be redundant having the parameter already called that way, has or contains wouldn't really fit since args is plural.

@angelofenoglio angelofenoglio merged commit 126f407 into master Sep 9, 2024
28 checks passed
@exequielrafaela exequielrafaela deleted the apply-bug-159 branch October 7, 2024 18:11
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.

5 participants