-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add Terraform outputs to status.atProvider #38
Conversation
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
@bobh66 thanks a lot for your contribution! Can you please investigate |
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
Hi @ytsarev - should be fixed now. Thanks! |
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.
Overall looks great, couple of suggestions
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
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.
Please make check-diff
again and commit :)
Hi @ytsarev - it appears that exposing a "generic" output object which can contain any type to the API is either impossible or beyond my limited understanding of go. I can get it to work functionally but the tools complain about exposing an interface{} to the API schema. I thought about reusing the ConnectionDetails format but that converts everything to bytes which end up base64 encoded. Unless you have suggestions for exposing a generic object as status output, I think I need to go back to the string format for everything. The Crossplane transform "convert" option can convert strings to int/bool/etc. That leaves tuples and maps in their stringified format, so anyone using Terraform will need to expose the specific output values that they want individually, instead of exposing an entire tuple or map. Any ideas how to make the output more generic while keeping the tools happy would be greatly appreciated. Thanks! |
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
@ytsarev lint works locally, does this need a retry? Thanks |
@bobh66 lint job also fails for me in another PR and also works locally - will investigate. Regarding 'generic' status, I recall you were initially using |
@ytsarev When I used preserve-unknown-fields I get this error from check-diff:
which I interpreted as saying that it is not allowed to have an object defined in the API without a specified schema. If there is a different/better way to do this I'd be happy to go back to that implementation, but I could not see a way forward from these errors. Thanks |
@bobh66 you indeed can't return interface but you should be able to return JSON and/or use https://pkg.go.dev/reflect to dynamically control the type... I will try to play with it soon |
@ytsarev What would you think about merging this as a first step, after any needed rework, and then in a separate PR we can refactor as needed to support tuples and objects? This change provides access to strings and numbers, which will be the majority of use cases. |
@bobh66 agreed! Can you please rebase with master again and we are good to merge |
@ytsarev - done. Thanks! |
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.
@bobh66 incredibly useful contribution, thanks a lot!
* Add Terraform outputs to status.atProvider Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com> * Add autogenerated code changes Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com> * Refactor code to use native types and add documentation to README file Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com> * Revert back to the stringified values, as interface cannot be exposed. Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
Signed-off-by: Bob Haddleton bob.haddleton@nokia.com
Description of your changes
Add the non-sensitive Terraform module outputs to the status.atProvider.outputs section of the Workspace object
Fixes #36
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
I tested these changes locally in a kind cluster using a mock Terraform module that outputs all 5 value types: