Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

w00x
Copy link

@w00x w00x commented Feb 20, 2024

No description provided.

module_function

def bearer_token
token = new_shark_token

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

faltaría algun cacheo

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, buscare alguna gema

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

Copy link
Author

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

Copy link

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',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

colocar URL real

Copy link
Author

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

Copy link

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

Copy link

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)

Copy link
Author

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

Copy link
Author

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
Copy link

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

@@ -0,0 +1,3 @@
{
"token": "eyJhbGciOiJSUzI1NiIsImtpZCI6ImFkNGYyMWRkLTgwZTItNGY2ZC1iOGY0LWE4OTE2ZDY0YTFhZiIsInR5cCI6IkpXVCJ9.eyJhZGRpdGlvbmFsX2luZm8iOnsiZW1haWwiOiJleGFtcGxlIiwiZnVsbF9uYW1lIjoiZXhhbXBsZSIsInVzZXJuYW1lIjoiZXhhbXBsZSJ9LCJleHAiOjQ3OTM4NzEyNzUsImlhdCI6MTY0MDI3MTI3NSwiaWRlbnRpdHkiOiJtcm46c2VnaW5mOmFkOnVzZXIvZXhhbXBsZSIsImlzcyI6ImZ1cnlfdGlnZXIiLCJzdWIiOiJleGFtcGxlIn0.OsfqgNjn52j_xxMRKNssDJQIgdVU4yVWVDnisI6yRGDyYw-hQ2-WnaHN9e5Obwpy6E0WDtdnt4Hk8nzk4QvMSQ"
Copy link

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

Copy link
Author

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

@w00x w00x requested review from arcticfalcon and lufunes February 22, 2024 18:04
Copy link

@lufunes lufunes left a 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?

@w00x
Copy link
Author

w00x commented Feb 23, 2024

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

@w00x w00x closed this Mar 1, 2024
@meli-cloudy meli-cloudy deleted the feature/FCXPINFRA-76-add-request-token branch March 1, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants