-
Notifications
You must be signed in to change notification settings - Fork 3
Show better ups details in MAR view #50
Show better ups details in MAR view #50
Conversation
be10c88
to
f975ef1
Compare
</div> | ||
|
||
<div class="col-md-6" ng-if="row.extendedAnnotationsType === 'unifiedpush'"> | ||
<h4 class="component-label section-label">Variants</h4> |
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.
I'm not thrilled with having code specific to a particular service hard-coded into the UI, I imagine we'd have a tough time getting this accepted upstream.
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.
@philbrookes that's something I was also concerned. I was asking that to @david-martin during a meeting and he told me that Craig and him discussed about that and this is what we can do initially.
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.
@philbrookes When discussing with @maleck13, he had no concerns about this being accepted upstream as it's a mobile specific view.
@sedroche @philbrookes how's the procedure here? am I supposed to send a PR for OpenShift upstream once this one is merged into our fork? |
@@ -70,6 +72,18 @@ | |||
return 0; | |||
}); | |||
|
|||
row.extendedAnnotationsType = row.mobileClient.metadata.annotations ? row.mobileClient.metadata.annotations[extendedAnnotationTypeKey] : undefined; |
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.
Is this to discern what service this is? We also have spec.externalMetadata.serviceName: ups
available on the service class.
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.
Yes, that's the purpose. I actually looked for something like that and couldn't find a key-value like that. Probably searched for wrong keywords.
Gonna delete that annotation in the operator side and in the UI.
thanks for the suggestion
@@ -70,6 +72,18 @@ | |||
return 0; | |||
}); | |||
|
|||
row.extendedAnnotationsType = row.mobileClient.metadata.annotations ? row.mobileClient.metadata.annotations[extendedAnnotationTypeKey] : undefined; | |||
|
|||
row.extendedAnnotations = _.pickBy(row.mobileClient.metadata.annotations, function(annotation, name){ |
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.
You can chain these if you want.
row.extendedAnnotations = _(row.mobileClient.metadata.annotations)
.pickBy()
.transform()
.value();
@aliok This PR will be part of the overall mobile services/binding work and a complete PR containing everything is what will be created upstream. |
Ok, changes applied. I will squash commits into a single commit when @pb82 verifies the PR. |
8eaea4d
to
1432a95
Compare
1432a95
to
8e7b319
Compare
Squashed and waiting for final approval |
https://issues.jboss.org/browse/AEROGEAR-2776
https://issues.jboss.org/browse/AEROGEAR-2777
Backend/configurator side: https://issues.jboss.org/browse/AEROGEAR-2850
Provision UPS on OpenShift first, using the branch below
See aerogear/ups-config-operator#38
Verification:
bower link
therebower link origin-webcommon
Some edge cases to test:
Screnshot:
If you don't want to start up the UPS side, add these annotations to mobile client: