-
Notifications
You must be signed in to change notification settings - Fork 523
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: add support for mssql_managed_instance #2393
Conversation
@tim775 I think this is ready for review - I would appreciate some feedback / some guidance on what else is left to do here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @grixxie this is great!
I left a few mostly style comments.
Also we need to add the usage parameters to the example usage file.
Let me know if you have time to make the changes, otherwise I'm happy to finish them off.
Cheers,
Tim
Thanks for this @tim775 , I will make these changes including the example usage file :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good! If you would, please update the golden file test to pick up the changes by running:
ARGS="--run TestMSSQLManagedInstance -v -update" make test_azure
Hey @tim775 , I'm still pretty new to this so I'd appreciate some help here - running I've followed the steps in CONTRIBUTING.md including the azure-credentials section but still no luck. I'm unsure whether it's a problem with my code changes or my local setup at this point - maybe you can help? Apologies, still getting into this 😆 |
@grixxie, no worries I actually ended up merging this with the intention of updating the golde file test after and ran into the same issue you're seeing now. It looks like the filters got messed up somehow, but it was a quick fix. |
@tim775 ah brilliant, this works for me now, thank you! |
addressing issue: #2063
infracost currently supports the "deprecated" sql_managed_instance -> add support for new mssql_managed_instance
any feedback appreciated