Log when using default authentication container name #1526
Conversation
|
@shulifink can you approve the following message:
For example, `Annotation 'authentication-container-name' not found. Using default value 'authenticator' |
orenbm
force-pushed
the
enhance-log-auth-container
branch
from
40f3729 to
f26efbb
Compare
orenbm
force-pushed
the
enhance-log-auth-container
branch
2 times, most recently
from
42bb21d to
3b0e4dc
Compare
orenbm
force-pushed
the
enhance-log-auth-container
branch
from
3b0e4dc to
c7f32e9
Compare
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| @@ -22,6 +22,9 @@ module AuthnK8s | |||
| # validation of ValidateApplicationIdentity | |||
| class ApplicationIdentity | |||
|
|
|||
| AUTHENTICATION_CONTAINER_NAME_ANNOTATION = "authentication-container-name" | |||
There was a problem hiding this comment.
Freeze mutable objects assigned to constants.
| @@ -22,6 +22,9 @@ module AuthnK8s | |||
| # validation of ValidateApplicationIdentity | |||
| class ApplicationIdentity | |||
|
|
|||
| AUTHENTICATION_CONTAINER_NAME_ANNOTATION = "authentication-container-name" | |||
| DEFAULT_AUTHENTICATION_CONTAINER_NAME = "authenticator" | |||
There was a problem hiding this comment.
Freeze mutable objects assigned to constants.
| default_authentication_container_name | ||
| end | ||
|
|
||
| def default_authentication_container_name |
There was a problem hiding this comment.
Authentication::AuthnK8s::ApplicationIdentity#default_authentication_container_name doesn't depend on instance state (maybe move it to another class?)
orenbm
force-pushed
the
enhance-log-auth-container
branch
from
c7f32e9 to
c2f44ee
Compare
| @@ -22,6 +22,9 @@ module AuthnK8s | |||
| # validation of ValidateApplicationIdentity | |||
| class ApplicationIdentity | |||
|
|
|||
| AUTHENTICATION_CONTAINER_NAME_ANNOTATION = "authentication-container-name".freeze | |||
There was a problem hiding this comment.
This is fine, but FYI you can also denote all string literals in the file as frozen by adding:
# frozen_string_literal: trueto the top of the file.
There was a problem hiding this comment.
oh nice, didn't know that. I'll change it and push again.
orenbm
force-pushed
the
enhance-log-auth-container
branch
from
c2f44ee to
248829d
Compare
|
@orenbm Just noticed this: I like this method of line breaking long strings, but in the above it will print at “notcall” instead of “not call”. I’ve been standardizing on always adding the space to the line above. |
|
thanks @jonahx, i fixed some other messages while i'm at it. |
If the annotation `authentication-container-name` is missing in
the host's annotations then we use a default value `authenticator`.
It can be helpful to the user to see this behaviour in the log. We already
log that an annotation was used to get this value
(e.g `Retrieved value of annotation {authn-k8s/authentication-container-name}`
but now we can also see that the default value is used.
orenbm
force-pushed
the
enhance-log-auth-container
branch
from
467750d to
2b9f679
Compare
|
Code Climate has analyzed commit 2b9f679 and detected 2 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 85.4% (0.0% change). View more on Code Climate. |
@orenbm - approved! |
Connected to conjurinc/dap-support#69
If the annotation
authentication-container-nameis missing inthe host's annotations then we use a default value
authenticator.It can be helpful to the user to see this behaviour in the log. We already
log that an annotation was used to get this value
(e.g
Retrieved value of annotation {authn-k8s/authentication-container-name}but now we can also see that the default value is used.