-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FCXPINFRA-76] Added Tiger token to requests #88
Conversation
lib/little_monster/tiger/auth.rb
Outdated
module_function | ||
|
||
def bearer_token | ||
token = new_shark_token |
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.
faltaría algun cacheo
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.
ok, buscare alguna gema
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.
fijate que en fury-api ya lo hace
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.
es que ese es el cache de rails
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.
una cosa a tener en cuenta es que no debería desaparecer el valor de la cache hasta no obtener el valor nuevo, aunque esté vencido. En rails nos pasó que se descartaba el valor almacenado en la cache (que aún era válido por varios minutos más) y luego no poder generar un nuevo token (o descargar un certificado) por un error temporal de red o la API
@@ -51,7 +51,9 @@ def default_config_values | |||
job_requests_retries: 4, | |||
job_requests_retry_wait: 1, | |||
heartbeat_execution_interval: 10, | |||
default_job_retries: -1 | |||
default_job_retries: -1, | |||
tiger_api_url: 'http://tiger', |
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.
colocar URL real
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.
en la configuración cuando la utilicen deberían agregar la url real
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.
el valor por defecto no debería ser la url real? para evitar problemas en los entornos donde no se configuró áun el valor
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.
sobretodo porque por defecto tenemos la funcionalidad habilitada (enable_tiger_token:true)
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.
ya que el api_url tampoco esta el original definido quise seguir esa linea, pero la puedo agregar
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.
creo que mejor la agregan al momento de usarla, ya que puede ser para Meli, FaaP, Sulamerica, etc... en implementación tal vez la obtengan de una ENV
"Bearer #{token}" if token | ||
end | ||
|
||
def new_shark_token |
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.
Hay instalaciones de LM API fuera de shark, quizas se puede agregar una config para no buscar el client credential de kubernetes y no ir a tiger (retornar directamente nil)
A futuro estas instalaciones deberian hacer login mediante el client credential de tiger
spec/mock/responses/tiger_token.json
Outdated
@@ -0,0 +1,3 @@ | |||
{ | |||
"token": "eyJhbGciOiJSUzI1NiIsImtpZCI6ImFkNGYyMWRkLTgwZTItNGY2ZC1iOGY0LWE4OTE2ZDY0YTFhZiIsInR5cCI6IkpXVCJ9.eyJhZGRpdGlvbmFsX2luZm8iOnsiZW1haWwiOiJleGFtcGxlIiwiZnVsbF9uYW1lIjoiZXhhbXBsZSIsInVzZXJuYW1lIjoiZXhhbXBsZSJ9LCJleHAiOjQ3OTM4NzEyNzUsImlhdCI6MTY0MDI3MTI3NSwiaWRlbnRpdHkiOiJtcm46c2VnaW5mOmFkOnVzZXIvZXhhbXBsZSIsImlzcyI6ImZ1cnlfdGlnZXIiLCJzdWIiOiJleGFtcGxlIn0.OsfqgNjn52j_xxMRKNssDJQIgdVU4yVWVDnisI6yRGDyYw-hQ2-WnaHN9e5Obwpy6E0WDtdnt4Hk8nzk4QvMSQ" |
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.
es necesario un token similar al real acá? teniendo en cuenta que es un repo público
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.
creo que no es necesario, lo cambio
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.
Dejo solo el comentario sobre la config por defecto. Teniendo en cuenta todos los LMs instalados, conviene dejar habilitado por defecto este feature y que explícitamente se apague? si está habilitado por defecto, deberíamos dejar la URL por defecto?
Agregare la url por defecto |
No description provided.