bug 1480988. Use project annotation to determine kibana url for ops namespaces #1968

jcantrill · 2017-08-25T17:28:50Z

This PR provides a fix to BZ1480988 to direct users to an operations instance of Kibana when the logging stack is deployed to have the ops logging cluster. This PR depends on openshift/openshift-ansible#5219

benjaminapetersen · 2017-08-25T17:40:20Z

app/scripts/directives/logViewer.js

@@ -401,6 +401,10 @@ angular.module('openshiftConsole')
                  return;
                }



Spelling of metadata:context.project.metdata vs context.project.metadata.

So most of our annotations are on a filter that, unfortunately, is now pulled in via a dependency from origin-web-common. We should probably add it there...

@benjaminapetersen fixed the spelling

thanks! so by annotation, i mean making a PR here simply to add the line:

var annotationMap = { // truncated loggingKibanaUrl: ['openshift.io/logging.kibana.url'] }

And then when that PR merges, can update this PR to use the filter:

url = "https://" + $scope.context.project.metadata.annotations[$filter('annotationName')('loggingKibanaUrl')];

jcantrill · 2017-08-25T20:11:34Z

Depends on openshift/origin-web-common/pull/158

spadgett · 2017-08-25T20:43:48Z

app/scripts/directives/logViewer.js

@@ -401,6 +401,10 @@ angular.module('openshiftConsole')
                  return;
                }

+                if($scope.context.project.metadata.annotations[$filter('annotationName')('loggingVisualURL') {
+                    url = 'https://' + $scope.context.project.metadata.annotations[$filter('annotationName')('loggingVisualURL');
+                }


@jcantrill It would be better to change APIDiscovery.getLoggingURL to take the project as a parameter and move the logic there. Then if we need the logging URL anywhere else, we don't forget to check the project annotation.

I'd suggest this code:

var projectLoggingURL = $filter('annotation')($scope.context.project, 'loggingVisualURL'); if (projectLoggingURL) { url = 'https://' + projectLoggingURL; }

benjaminapetersen · 2017-08-28T17:02:51Z

@jcantrill grunt build & commit the dist as well & travis should pass.

jcantrill · 2017-08-28T19:57:55Z

@spadgett Updated correctly?

spadgett · 2017-08-28T20:20:00Z

app/scripts/services/discovery.js

+      getLoggingURL: function(project) {
+        if(project.metadata.annotations[$filter('annotationName')('loggingUIHostname')]) {
+           return 'https://' + project.metadata.annotations[$filter('annotationName')('loggingUIHostname')];
+        }


I think this is easier to read (untested)

var loggingUIHostname = $filter('annotation')(project, 'loggingUIHostname'); if (loggingUIHostname) { return 'https://' + loggingUIHostname; }

spadgett · 2017-08-28T20:20:39Z

Will require an origin-web-common release and dist files

spadgett · 2017-08-28T23:06:53Z

app/scripts/services/discovery.js

    return {
      // Simulate asynchronous requests for now. If these are ever updated to call to a discovery
      // endpoint, we need to make sure to trigger a digest loop using (or update all callers).
-      getLoggingURL: function() {
+      getLoggingURL: function(project) {
+        loggingUIHostname = $filter('annotationName')(project, 'loggingUIHostname');


Missing var.

var loggingUIHostname = $filter('annotationName')(project, 'loggingUIHostname');

spadgett · 2017-08-28T23:06:57Z

app/scripts/services/discovery.js

+      getLoggingURL: function(project) {
+        loggingUIHostname = $filter('annotationName')(project, 'loggingUIHostname');
+        if(loggingUIHostname) {
+           return 'https://' + loggingUIHostname;


Sorry @jcantrill missed this on last review, but this needs to be wrapped in a $q.when since the method returns a promise.

return $q.when('https://' + loggingUIHostname);

Or you can change it to have one return.

var loggingURL = LOGGING_URL; var loggingUIHostname = $filter('annotationName')(project, 'loggingUIHostname'); if(loggingUIHostname) { loggingURL = 'https://' + loggingUIHostname; } return $q.when(loggingURL);

jcantrill · 2017-08-29T12:41:19Z

@spadgett I made the requested changes but should I expect all the changes found in dist and dist.java ?

spadgett · 2017-08-29T12:44:13Z

No, you shouldn't have that. Try

$ hack/clean-asssets.sh
$ hack/install-assets.sh
$ grunt build

spadgett · 2017-08-29T12:45:33Z

@liggitt We post the user's access token to the logging URL. Just confirming that the annotation can only be changed by admins and that this is safe.

spadgett

@jcantrill Can you make sure you've tested the changes? You should be able to add the annotation to the namespace manually if you need.

The origin-web-common change has merged if you rebase.

spadgett · 2017-08-29T12:48:04Z

app/scripts/services/discovery.js

-        return $q.when(LOGGING_URL);
+      getLoggingURL: function(project) {
+        var loggingURL = LOGGING_URL;
+        var loggingUIHostname = $filter('annotationName')(project, 'loggingUIHostname');


$filter('annotation')

…amespaces

jcantrill · 2017-08-29T13:41:42Z

@spadgett tested.... works like a champ...

spadgett · 2017-08-29T13:52:46Z

[merge]

openshift-bot · 2017-08-29T14:07:15Z

Evaluated for origin web console merge up to bac757f

spadgett · 2017-08-29T14:14:35Z

@jcantrill Looks like this needs to be rebased again :/

openshift-bot · 2017-08-29T16:00:33Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/91/) (Base Commit: 72f8452) (PR Branch Commit: bac757f)

jcantrill requested review from benjaminapetersen and spadgett August 25, 2017 17:28

benjaminapetersen reviewed Aug 25, 2017

View reviewed changes

jcantrill force-pushed the bz1480988_fix_ops_kibana branch from 75c2e92 to 8d1f6f2 Compare August 25, 2017 20:14

spadgett reviewed Aug 25, 2017

View reviewed changes

jcantrill force-pushed the bz1480988_fix_ops_kibana branch 3 times, most recently from 370a48e to a7d19ac Compare August 28, 2017 19:57

spadgett reviewed Aug 28, 2017

View reviewed changes

jcantrill force-pushed the bz1480988_fix_ops_kibana branch from a7d19ac to b0abba9 Compare August 28, 2017 20:28

spadgett requested changes Aug 28, 2017

View reviewed changes

jcantrill force-pushed the bz1480988_fix_ops_kibana branch from b0abba9 to 707e723 Compare August 29, 2017 12:40

spadgett requested changes Aug 29, 2017

View reviewed changes

bug 1480988. Use project annotation to determine kibana url for ops n…

bac757f

…amespaces

jcantrill force-pushed the bz1480988_fix_ops_kibana branch from 707e723 to bac757f Compare August 29, 2017 13:13

spadgett approved these changes Aug 29, 2017

View reviewed changes

openshift-bot merged commit 04d694f into openshift:master Aug 29, 2017

jcantrill deleted the bz1480988_fix_ops_kibana branch August 29, 2017 17:35

jcantrill mentioned this pull request Aug 30, 2017

bug 1480988. Link ops pods to ops kibana #1990

Merged

		@@ -401,6 +401,10 @@ angular.module('openshiftConsole')
		return;
		}

bug 1480988. Use project annotation to determine kibana url for ops namespaces #1968

bug 1480988. Use project annotation to determine kibana url for ops namespaces #1968

Uh oh!

Conversation

jcantrill commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcantrill commented Aug 25, 2017

Uh oh!

spadgett Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen commented Aug 28, 2017

Uh oh!

jcantrill commented Aug 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spadgett commented Aug 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcantrill commented Aug 29, 2017

Uh oh!

spadgett commented Aug 29, 2017

Uh oh!

spadgett commented Aug 29, 2017

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcantrill commented Aug 29, 2017

Uh oh!

spadgett commented Aug 29, 2017

Uh oh!

openshift-bot commented Aug 29, 2017

Uh oh!

spadgett commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-bot commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

spadgett Aug 25, 2017 •

edited

Loading

spadgett commented Aug 29, 2017 •

edited

Loading

openshift-bot commented Aug 29, 2017 •

edited

Loading