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

Use HasSuffix to check for suffix #1505

Merged
merged 8 commits into from
Feb 4, 2025
Merged

Conversation

amangale
Copy link
Contributor

@amangale amangale commented Jan 20, 2025

For checking the existence of a workspace, the existing regular expression match is not required and is prone to failure. Given that the workspace name is essentially a suffix, the idiomatic way would be to use HasSuffix.

The existing regular expression match is not required and is prone to failure. Given that the workspace name is essentially a suffix, the idiomatic way would be to use `HasSuffix`.
@amangale amangale requested a review from denis256 as a code owner January 20, 2025 10:13
The function `nameMatchesWorkspace` is no longer required.
This function is no longer required.
@james03160927
Copy link
Contributor

Hi @amangale, I believe this change might not be backward compatible since it alters the existing behavior. Could you elaborate on why this change is necessary and clarify what you mean by "prone to failure"?

@amangale
Copy link
Contributor Author

Hi @amangale, I believe this change might not be backward compatible since it alters the existing behavior. Could you elaborate on why this change is necessary and clarify what you mean by "prone to failure"?

Hi @james03160927 -
The earlier code was looking for the ws name at the end of the string which is a suffix.
We recently upgraded to TF v1.10 and Terragrunt v0.72 and modified the logging. This broke WorkspaceSelectOrNewE since the output of terraform workspace list was a different string with the ws name at the end.
I have this sample code: https://go.dev/play/p/g8JiafkjXmU
It demonstrates backward compatibility. If there is a specific use case you can share, I can modify the PR to accommodate it.

@james03160927
Copy link
Contributor

If we can modify the code to support the same functionality while also accommodating new use cases with the upgrade, it will ensure that existing users do not encounter sudden errors, especially in scenarios where their automatic setups update to the latest Terratest version.

@amangale
Copy link
Contributor Author

If we can modify the code to support the same functionality while also accommodating new use cases with the upgrade, it will ensure that existing users do not encounter sudden errors, especially in scenarios where their automatic setups update to the latest Terratest version.

Hi @james03160927 -
In the playground link, all the existing test cases are passing. This ensures backward compatibility and none of the existing functionality will break because of this change. Is there any other way we can ensure complete and total backward compatibility. Let me know, I would be happy to try that.

@amangale
Copy link
Contributor Author

amangale commented Jan 23, 2025

@james03160927 - If it helps, here is the change in workspace list I am referring to:
(with terragrunt version v0.72.0 and terraform version v1.10.4)
12:57:34.856 STDOUT terraform: default
12:57:34.857 STDOUT terraform: workspace01-dasdasd
12:57:34.857 STDOUT terraform: workspace02-sdfdsfds
12:57:34.857 STDOUT terraform: workspace03-cvxcvxc
12:57:34.857 STDOUT terraform: workspace04-qweqwe
(with terragrunt version v0.58.2 and terraform version v1.9.8)
* default
sample01workspace01-dasdasd
sample02workspace01-gdfgghgfh
sample03workspace01-ouiouioiu
Given that the ws name is always the suffix, I have proposed this change.

@bharat-devops
Copy link

Hi @james03160927 ,@amangale,

As a temporary solution for latest terragrunt version, where it spits out extra values at stdout during workspace selection .
We can use either terragrunt workspace list --terragrunt-forward-tf-stdout at command line or set it at Env - export TERRAGRUNT_FORWARD_TF_STDOUT=true

@james03160927
Copy link
Contributor

Thanks for the clarification. Will trigger the test pipeline and see if it passes all the other tests.
In the meantime, can you click the "update branch" button to merge with the latest change in main?

@james03160927
Copy link
Contributor

It's failing the precommit test

goimports................................................................Failed
- hook id: goimports
- files were modified by this hook

@amangale
Copy link
Contributor Author

@james03160927 -
The PR has been updated and the imports have been fixed.

@james03160927
Copy link
Contributor

Triggering test again.

@james03160927
Copy link
Contributor

Failing tests doesn't seem to be related to your change. LGTM.

Copy link
Contributor

@james03160927 james03160927 left a comment

Choose a reason for hiding this comment

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

LGTM

@denis256 denis256 merged commit 23563d0 into gruntwork-io:main Feb 4, 2025
2 checks passed
@amangale amangale deleted the patch-4 branch February 4, 2025 19:01
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.

4 participants