Remove hyphen from openid-connect section of the endpoints api #3241

npalaska · 2023-02-09T16:05:03Z

Addresses issue #3240
Originally asked here

Commit message:

Remove hyphen from openid-connect section of the endpoints API

Getting rid of the excess quotes and brackets would make this easier to read
It also makes easier to reference in Dashboard javascript code when there is
no hyphen involved (endpoints.openid instead of endpoints['openid-connect'])

dbutenhof

I don't know that issuer needed to change to server (although I suspect it's more accurate as we have the broker address but we expect an different SSO "issuer") ... but I don't object. 😄

dbutenhof · 2023-02-09T16:18:13Z

By the way; make sure your merge commit has a better explanation than just that.

webbnh

👍

npalaska · 2023-02-09T16:42:20Z

By the way; make sure your merge commit has a better explanation than just that.

I updated the description with a commit message.

portante

Why is this needed?

webbnh · 2023-02-09T17:31:23Z

Why is this needed?

I'm sorry that the references in the description weren't sufficient.

The answer is that the presence of the hyphen in the name, openid-connect, makes the identifier awkward to use in the Dashboard's Javascript code. (Not impossible, not even hard, just awkward and ugly.) So, if we rename it here, it makes life better there.

portante

Is this the kind of code change we want to be making now before we have verified the server is deployed in staging? It seems that given our time constraints to deploy by mid-March we should spend on time on other kinds of changes.

portante · 2023-02-09T17:32:04Z

lib/pbench/test/unit/server/test_endpoint_configure.py

@@ -140,15 +140,15 @@ def check_config(self, client, server_config, host, my_headers={}):

        try:
            oidc_client = server_config.get("openid-connect", "client")


FWIW, these lists were in sorted order.

I support keeping the list in sorted order.

On another note, @portante, when commenting on multiple lines, you can drag-select to highlight the range, and doing so has two effects: first, in the "Conversation" display, your comment will be preceded by the code from the range (instead of the default three or so lines), and, second and more importantly, your comment will be displayed after the range (in this instance, it would have made clear what you meant by "these lists").

portante · 2023-02-09T17:33:19Z

lib/pbench/server/api/resources/endpoint_configure.py

        except (NoOptionError, NoSectionError):
            pass
        else:
-            endpoints["openid-connect"] = {
+            endpoints["openid"] = {


Perhaps "OpenID Connect" could be shortened to oidc if we really need it that short?

Brevity is not the issue; what we're trying to do is to make it be a valid Javascript identifier, which means removing the hyphen.

webbnh

Is this the kind of code change we want to be making now before we have verified the server is deployed in staging? It seems that given our time constraints to deploy by mid-March we should spend on time on other kinds of changes.

That question is constructive looking forward; however, as regards this particular change, the work is already done, and I do not think that accepting this change at this point will adversely affect the Staging server deployment (and, I believe that it will ease the addition of Keycloak-based authentication).

webbnh · 2023-02-09T17:39:28Z

lib/pbench/test/unit/server/test_endpoint_configure.py

@@ -140,15 +140,15 @@ def check_config(self, client, server_config, host, my_headers={}):

        try:
            oidc_client = server_config.get("openid-connect", "client")


I support keeping the list in sorted order.

On another note, @portante, when commenting on multiple lines, you can drag-select to highlight the range, and doing so has two effects: first, in the "Conversation" display, your comment will be preceded by the code from the range (instead of the default three or so lines), and, second and more importantly, your comment will be displayed after the range (in this instance, it would have made clear what you meant by "these lists").

webbnh · 2023-02-09T17:40:56Z

lib/pbench/server/api/resources/endpoint_configure.py

        except (NoOptionError, NoSectionError):
            pass
        else:
-            endpoints["openid-connect"] = {
+            endpoints["openid"] = {


Brevity is not the issue; what we're trying to do is to make it be a valid Javascript identifier, which means removing the hyphen.

portante · 2023-02-09T17:49:55Z

Is this the kind of code change we want to be making now before we have verified the server is deployed in staging? It seems that given our time constraints to deploy by mid-March we should spend on time on other kinds of changes.

That question is constructive looking forward; however, as regards this particular change, the work is already done, and I do not think that accepting this change at this point will adversely affect the Staging server deployment (and, I believe that it will ease the addition of Keycloak-based authentication).

I don't believe the principle of accepting something because it is already done is a good principle to follow for a project. That line of thinking means that any PR submitted should be accepted because it is done.

In this case, we don't need to be accepting ANYTHING for OpenID Connect support. If folks want to build up a series of commits for OpenID Connect on a separate branch to be merged later, great.

But we need to remain focused.

npalaska · 2023-02-09T17:58:31Z

I don't believe the principle of accepting something because it is already done is a good principle to follow for a project. That line of thinking means that any PR submitted should be accepted because it is done.

In this case, we don't need to be accepting ANYTHING for OpenID Connect support. If folks want to build up a series of commits for OpenID Connect on a separate branch to be merged later, great.

But we need to remain focused.

@portante If you want to have main only focus on things we need to do other than OpenID-related changes and then deploy it on the staging and after that we can merge all the OpenID-related changes onto the main, I am fine with it. We can create another branch for the time being for all the OpenID-related changes that we can consolidate on the main later.

npalaska · 2023-02-09T19:02:55Z

FWIW, this will now land in openid-connect branch instead of main, that's where all the openid-connect related changes will go for a while until we have a staging server deployed.

webbnh

👍

Since this is no longer targeting main it is not a concern.

…ibuted-system-analysis#3241) Remove hyphen from openid-connect section of the endpoints API - Getting rid of the excess quotes and brackets would make this easier to read - It also makes it easier to reference in Dashboard javascript code when there is no hyphen involved (endpoints.openid instead of endpoints['openid-connect'])

Remove hyphen from openid-connect section of the endpoints API - Getting rid of the excess quotes and brackets would make this easier to read - It also makes it easier to reference in Dashboard javascript code when there is no hyphen involved (endpoints.openid instead of endpoints['openid-connect'])

…ibuted-system-analysis#3241) Remove hyphen from openid-connect section of the endpoints API - Getting rid of the excess quotes and brackets would make this easier to read - It also makes it easier to reference in Dashboard javascript code when there is no hyphen involved (endpoints.openid instead of endpoints['openid-connect'])

Remove hyphen from openid-connect section of the endpoints API - Getting rid of the excess quotes and brackets would make this easier to read - It also makes it easier to reference in Dashboard javascript code when there is no hyphen involved (endpoints.openid instead of endpoints['openid-connect'])

Remove hyphen from openid-connect section of the endpoints api

809b553

npalaska self-assigned this Feb 9, 2023

npalaska added Server Users Of and relating to working with users. labels Feb 9, 2023

npalaska added this to the v0.72 milestone Feb 9, 2023

npalaska linked an issue Feb 9, 2023 that may be closed by this pull request

Renameopenid-connect variable in the endpoints api to openid or openid_connect? #3240

Closed

dbutenhof previously approved these changes Feb 9, 2023

View reviewed changes

webbnh previously approved these changes Feb 9, 2023

View reviewed changes

webbnh mentioned this pull request Feb 9, 2023

Enable OIDC redirect in dashboard #3233

Merged

webbnh mentioned this pull request Feb 9, 2023

Add OIDC user to the functional test #3235

Merged

npalaska requested a review from portante February 9, 2023 17:18

portante requested changes Feb 9, 2023

View reviewed changes

portante previously requested changes Feb 9, 2023

View reviewed changes

webbnh reviewed Feb 9, 2023

View reviewed changes

re-order variables

4ae3ea6

npalaska dismissed stale reviews from dbutenhof and webbnh via 4ae3ea6 February 9, 2023 18:17

npalaska changed the base branch from main to openid-connect February 9, 2023 18:42

webbnh approved these changes Feb 9, 2023

View reviewed changes

dbutenhof approved these changes Feb 9, 2023

View reviewed changes

npalaska merged commit f04b8fa into distributed-system-analysis:openid-connect Feb 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove hyphen from openid-connect section of the endpoints api #3241

Remove hyphen from openid-connect section of the endpoints api #3241

npalaska commented Feb 9, 2023 •

edited by dbutenhof

Loading

dbutenhof left a comment •

edited

Loading

dbutenhof commented Feb 9, 2023

webbnh left a comment

npalaska commented Feb 9, 2023

portante left a comment

webbnh commented Feb 9, 2023

portante left a comment

portante Feb 9, 2023

webbnh Feb 9, 2023

portante Feb 9, 2023

webbnh Feb 9, 2023

webbnh left a comment

webbnh Feb 9, 2023

webbnh Feb 9, 2023

portante commented Feb 9, 2023

npalaska commented Feb 9, 2023

npalaska commented Feb 9, 2023

webbnh left a comment

		@@ -140,15 +140,15 @@ def check_config(self, client, server_config, host, my_headers={}):

		try:
		oidc_client = server_config.get("openid-connect", "client")

Remove hyphen from openid-connect section of the endpoints api #3241

Remove hyphen from openid-connect section of the endpoints api #3241

Conversation

npalaska commented Feb 9, 2023 • edited by dbutenhof Loading

dbutenhof left a comment • edited Loading

Choose a reason for hiding this comment

dbutenhof commented Feb 9, 2023

webbnh left a comment

Choose a reason for hiding this comment

npalaska commented Feb 9, 2023

portante left a comment

Choose a reason for hiding this comment

webbnh commented Feb 9, 2023

portante left a comment

Choose a reason for hiding this comment

portante Feb 9, 2023

Choose a reason for hiding this comment

webbnh Feb 9, 2023

Choose a reason for hiding this comment

portante Feb 9, 2023

Choose a reason for hiding this comment

webbnh Feb 9, 2023

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

webbnh Feb 9, 2023

Choose a reason for hiding this comment

webbnh Feb 9, 2023

Choose a reason for hiding this comment

portante commented Feb 9, 2023

npalaska commented Feb 9, 2023

npalaska commented Feb 9, 2023

webbnh left a comment

Choose a reason for hiding this comment

npalaska commented Feb 9, 2023 •

edited by dbutenhof

Loading

dbutenhof left a comment •

edited

Loading