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: log when tenant not associated to common variable template #792

Merged
merged 4 commits into from
Oct 4, 2024

Conversation

mjhilton
Copy link
Contributor

@mjhilton mjhilton commented Oct 3, 2024

Add better detection and a better error message when trying to provide a value for a Common Variable Template, but the Tenant has not yet been connected to a Project that includes the Library Set that defines the template.

fixes #790

How to review

  • Detection logic feels pretty unstreamlined and verbose; happy for suggestions on more canonical golang or a better approach.
  • I don't understand why the compiler warning requries error messages to start with a lower-case, that feels a bit uncivilized to me. Am I doing the error message approach wrong?

Copy link
Contributor

@domenicsim1 domenicsim1 left a comment

Choose a reason for hiding this comment

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

Overall logic looks good.
Put some comments down on some Go things.

@domenicsim1
Copy link
Contributor

domenicsim1 commented Oct 3, 2024

Detection logic feels pretty unstreamlined and verbose; happy for suggestions on more canonical golang or a better approach.

I would like to know more about what you mean here.

I don't understand why the compiler warning requries error messages to start with a lower-case, that feels a bit uncivilized to me. Am I doing the error message approach wrong?

My suggestion here would be to return a diag.diagnostic that we append to resp . You can do so with diag.NewErrorDiagnostic(summary, detail) Errors are designed for Go, and target "developer" consummation when they are called with panic(err) or being logged. In this case having them lower-case fits with how panic and log formats the errors in Go. They are not designed for user facing errors but terraform diag is.

@mjhilton
Copy link
Contributor Author

mjhilton commented Oct 4, 2024

I would like to know more about what you mean here.

Flipping the logical "polarity" of the checks between the three conditions feels a little off. By that, I mean that in the first one - checking if the Tenant is connected to any Projects - I'm returning an error condition; then, I'm looking for a positive condition in the loop that searches for the template and returning nil if it's good; then I'm going back to returning an error condition again. To illustrate with some pseudocode, I would usually expect this style of return-early method to follow a pattern like this:

if (a thing) 
  return true;

if (another thing) 
  return true;

return false

but instead I'm doing

if (a thing)
  return false

if (another thing)
  return true

return false

So it's fine, it just doesn't feel nice. Happy for any suggestion on how to improve this; I couldn't think of anything.

@mjhilton
Copy link
Contributor Author

mjhilton commented Oct 4, 2024

Also, what are these iffy-loops and where is LINQ? 🤪

@mjhilton mjhilton merged commit 6e97b3c into main Oct 4, 2024
22 checks passed
@mjhilton mjhilton deleted the matth/790-variable-availability-log-message branch October 4, 2024 01:04
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.

Can't create Tenant Common Variable without link Tenant to a Project
2 participants