search

aerogearcatalog / unifiedpush-apb

Apache License 2.0
6 stars 21 forks source link

feat: annotation of mobile client #82

Closed psturc closed 5 years ago

psturc commented 5 years ago

Motivation

https://issues.jboss.org/browse/AEROGEAR-8138

pb82 commented 5 years ago

@psturc Not sure if it's the same thing but the ups operator already annotates the mobile client: https://github.com/aerogear/ups-config-operator/blob/master/pkg/configOperator/configOperator.go#L403

psturc commented 5 years ago

@pb82 thanks Peter for this comment, you're right! Operator should add the annotation, but currently it's not present in client after binding. So I checked the logs and found this:

2018/11/26 15:44:12 MobileClient.mobile.k8s.io "myapp" is invalid: []: Invalid value: map[string]interface
...
validation failure list:
--
  | spec.appIdentifier in body should match '([\w-])'
spec.clientType in body should match 'cordova|iOS|android|xamarin'

This is caused by missing appId & clientType - we removed it from MDC some time ago. I think this could be easily fixed.

@wei-lee @pb82 if you agree, I will send PR with fix to ups operator and close this PR.

pb82 commented 5 years ago

@psturc Yes that sounds good. The operator also takes care of removing the annotations: https://github.com/aerogear/ups-config-operator/blob/master/pkg/configOperator/configOperator.go#L312

The only reason to do the annotations in the APB would be if you want to keep it uniform to how the other APBs handle it. What do you think @wei-lee ?

wei-lee commented 5 years ago

sorry it took so long to verify it. I had some problems with my minishift VM.

I verified the change with this PR. It does work as expected, however, as @pb82 pointed out, looks like the ups operator is already doing the same, I ended up with 2 URLs with the same value on the screen, one is called "URL" and the other is called "UPS Admin Console URL".

So I think maybe we just rename the "URL" label to "UPS Admin Console URL" in the UPS operator here?

wei-lee commented 5 years ago

@psturc looks like if you update the MobileClient resource with the latest version of the crd file from MDC, it will work.

We probably should update the ups operator to use the latest version of the mobile client crd. You need to update it in this repo: https://github.com/aerogear/mobile-crd-client.

Once it's done, we can probably update MDC to use that repo as dependency as well.