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

Skip Terraform version checks when not needed #613

Merged
merged 3 commits into from
Jul 4, 2021

Conversation

0x2b3bfa0
Copy link
Member

Terraform should not be a dependency for invocations that don't include --cloud or --tf-resource options. Should fix #612.

Terraform should not be a dependency for invocations that don't include
--cloud or --tf-resource
@0x2b3bfa0 0x2b3bfa0 added bug Something isn't working cml-runner Subcommand labels Jun 28, 2021
@0x2b3bfa0 0x2b3bfa0 self-assigned this Jun 28, 2021
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 28, 2021 13:43 Inactive
Copy link
Contributor

@DavidGOrtega DavidGOrtega left a comment

Choose a reason for hiding this comment

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

It's not enough. It should be check also in the termination

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 28, 2021 14:29 Inactive
@0x2b3bfa0
Copy link
Member Author

@DavidGOrtega, is there any scenario where the termination code is executed without reaching first the startup code?

@DavidGOrtega
Copy link
Contributor

Non cloud runner do not have tf-resource however when terminating is going to try to destroy

@0x2b3bfa0
Copy link
Member Author

🤦🏼 So true!

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 28, 2021 15:08 Inactive
@0x2b3bfa0
Copy link
Member Author

@DavidGOrtega, please take a look at the latest changes. I haven't tested them yet, but it looks like that should do the trick. 🙊

Copy link
Contributor

@DavidGOrtega DavidGOrtega left a comment

Choose a reason for hiding this comment

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

👍 lgtm however I will try it first. You dont have to!

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 30, 2021 17:56 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 30, 2021 18:38 Inactive
Additionally, refactor block to use true conditionals instead of && shortcut conditionals.
@0x2b3bfa0 0x2b3bfa0 force-pushed the check-terraform-version-when-needed branch from 0572a3a to a0e0f57 Compare June 30, 2021 20:05
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 30, 2021 20:05 Inactive
@DavidGOrtega DavidGOrtega merged commit 4496976 into master Jul 4, 2021
@DavidGOrtega DavidGOrtega deleted the check-terraform-version-when-needed branch July 4, 2021 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cml-runner Subcommand
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform dependency should be only a must if the resource is set
3 participants