Skip to content

Use HasSuffix to check for suffix#1505

Merged
denis256 merged 8 commits intogruntwork-io:mainfrom
amangale:patch-4
Feb 4, 2025
Merged

Use HasSuffix to check for suffix#1505
denis256 merged 8 commits intogruntwork-io:mainfrom
amangale:patch-4

Conversation

@amangale
Copy link
Copy Markdown
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.
@james00012
Copy link
Copy Markdown
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
Copy Markdown
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.

@james00012
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

@james00012
Copy link
Copy Markdown
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?

@james00012
Copy link
Copy Markdown
Contributor

It's failing the precommit test

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

@amangale
Copy link
Copy Markdown
Contributor Author

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

@james00012
Copy link
Copy Markdown
Contributor

Triggering test again.

@james00012
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

@james00012 james00012 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
@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