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

Service binding supporting deployment config #1077

Merged
merged 3 commits into from
Sep 17, 2021
Merged

Service binding supporting deployment config #1077

merged 3 commits into from
Sep 17, 2021

Conversation

wtrocki
Copy link
Collaborator

@wtrocki wtrocki commented Sep 16, 2021

What this does

We are adding deploymentConfig support by extra flag.

Why flag instead of showing both?
Deployment config is typically problematic with binding (extra rollouts are needed and we cannot make sure that binding will be valid). We needed to provide extra documentation etc. to support that - adding it by default will cause confusion and problems.
It will also break our existing guides :/

Notes

This change doesn't introduce any multiservice aritecture.
This is going to be done in separate feature branch done by @rkpattnaik780
I have prepared layer for that refactoring but we need to work on top of the changes that Rama has right now.

Verification

[wtrocki@graphapi app-services-cli (support-bind ✗)]$ rhoas cluster bind --deployment-config 
? Select Kafka instance to connect: pete
Namespace not provided. Using wtrocki namespace
? Please select application you want to connect with example
Binding "pete" with "example" app
? Do you want to continue? Yes
Using ServiceBinding Operator to perform binding
✔️ Binding pete with example app succeeded

@wtrocki
Copy link
Collaborator Author

wtrocki commented Sep 16, 2021

WIP. FYI @rkpattnaik780

@wtrocki
Copy link
Collaborator Author

wtrocki commented Sep 17, 2021

@craicoverflow just review needed. Verification not possible - SBO operator seems to be broken

@@ -26,7 +26,7 @@ require (
github.com/redhat-developer/app-services-sdk-go/kafkamgmt v0.4.0
github.com/redhat-developer/app-services-sdk-go/registryinstance v0.3.1
github.com/redhat-developer/app-services-sdk-go/registrymgmt v0.2.0
github.com/redhat-developer/service-binding-operator v0.8.0
github.com/redhat-developer/service-binding-operator v0.9.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't v0.10.0 announced? Maybe that could resolve it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

0.10.0 is actually the version of the cluster that breaks things.
0.10.0 also doesn't seem to compile redhat-developer/service-binding-operator#1022

I have verified this for deployment config. So service binding is created and command works.
but we cannot verify this end to end. I think we should merge this knowing that command works as it should

Copy link
Contributor

@craicoverflow craicoverflow left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/cluster/serviceBinding.go Outdated Show resolved Hide resolved
}
logger.Debug("Service binding Operator not available. Will use SDK option for binding")
return useSDKForBinding(clients, sb)
return localizer.MustLocalizeError("cluster.serviceBinding.operatorMissing", localize.NewEntry("Error", err))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return localizer.MustLocalizeError("cluster.serviceBinding.operatorMissing", localize.NewEntry("Error", err))
return localizer.MustLocalizeError("cluster.serviceBinding.operatorMissing", localize.NewEntry("Error", err))

Passing in the err object to localizer wil convert it to a string, and you cannot do any casting/type checking of the error from thereon in. It's recommended to preserve the error object.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we discussed that yesteday that we going to map errors specifically for providing more reliable info.
So we should probably:

  • Log error that we recognize
  • Return error to be printed again with more generic/non i18n content

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes - in the commands it makes sense to map errors and preserving the error object does not matter too much. However in an SDK scenario (like this one) I think it is important to preserve it because the place it is being used is able act different based on the error type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case error will be cryptic - 404 kafka API doesn't exist or missing auth token. Do you suggest to return original error only?

Copy link
Contributor

Choose a reason for hiding this comment

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

No it can be wrapped like this:

return fmt.Errorf("your reason: %w", err)

%w means wrap the error.

pkg/cluster/serviceBinding.go Outdated Show resolved Hide resolved
pkg/localize/locales/en/cmd/cluster_bind.en.toml Outdated Show resolved Hide resolved
pkg/localize/locales/en/cmd/cluster_bind.en.toml Outdated Show resolved Hide resolved
pkg/cmd/cluster/bind/bind.go Outdated Show resolved Hide resolved
@rkpattnaik780
Copy link
Contributor

The error messages need formatting at the last line.

Screenshot from 2021-09-17 18-15-52

Screenshot from 2021-09-17 18-28-31

Also for the latter error message, not sure if it was expected to fail, I could do a bind after it.

@wtrocki
Copy link
Collaborator Author

wtrocki commented Sep 17, 2021

@rkpattnaik780 That is mostly because of logic - it lets you to select kafka that you do not have on the cluster.
I have left comment to solve that problem - once we move to binding we should read Kafka and Registry connections from cluster and specify their names instead of reading them from backend

@wtrocki wtrocki marked this pull request as ready for review September 17, 2021 14:39
@wtrocki
Copy link
Collaborator Author

wtrocki commented Sep 17, 2021

Blocked by:
redhat-developer/service-binding-operator#1025

My quess is that we going to merge and release it so we can rebase and continue refactoring it for multiservice architecture

@wtrocki wtrocki merged commit cbb9b64 into main Sep 17, 2021
@wtrocki wtrocki deleted the support-bind branch September 17, 2021 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants