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

fix: confighelper.Trilean to show as boolean #244

Closed
wants to merge 1 commit into from

Conversation

cpwc
Copy link

@cpwc cpwc commented Jun 6, 2024

Users do not need to know what's confighelper.Trilean similar to hashicorp/packer#8673 to avoid confusion.

@cpwc cpwc requested a review from a team as a code owner June 6, 2024 09:14
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Hi @cpwc,

Thanks for opening this PR, I must ask however, why is the name confighelper.Trilean since the type is defined in the config package here?
I'm not sure I understand what is the fix for.

@cpwc
Copy link
Author

cpwc commented Jun 6, 2024

@lbajolet-hashicorp
Copy link
Contributor

I see.

Not sure we should handle every alias possible here, the approach to determine the type should probably be revisited then.
If this is a major pain point I can see us accepting this in the meantime, but the fact that we're missing something as simple as a package alias means this is in sore need of refactoring.

@nywilken thoughts?

@cpwc
Copy link
Author

cpwc commented Jun 21, 2024

@lbajolet-hashicorp @nywilken alias might change but the actual type won't change. A potential change we can look at is see if it contains Trilean?

@lbajolet-hashicorp
Copy link
Contributor

Hi @cpwc,

It's another heuristic that I think will be sufficient in most cases, but can still fall short in some cases.
I would like for us to take our time and address this in a way that will be rather robust if possible.

I've opened a PR on the AWS plugin to scrub the references to confighelper.Trilean over there, so the name is processed adequately for this plugin, if you have experienced this on other plugins I would suggest opening an issue there if possible in order to fix usage.

I'll convert this PR into an issue so we can track an plan for this work directly in the SDK, hopefully we can use type analysis to figure out what we're dealing with.

@cpwc cpwc deleted the fix-trilean branch June 21, 2024 14:29
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.

2 participants