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

feat: added logs to errors imports #259

Merged
merged 3 commits into from
May 17, 2024
Merged

Conversation

BetoFandino
Copy link
Contributor

Logs in errors imports

Description

This change is due to the difficulty in tracking errors in some imports. They will help to locate faster any errors or warnings that may occur that may hinder testing and debugging.

Useful information to include:

  • Which user roles will this change impact: Developer.

Checklist for Merge

  • [ x] Tested in a remote environment / NA
  • [x ] Updated documentation / NA
  • [ x] Rebased master/main / NA
  • [x ] Squashed commits / NA

@MaferMazu
Copy link
Contributor

Thanks for this @BetoFandino.
In #254, we already added social-auth-core as a requirement for this plugin to avoid the try-except errors related to social_core. But we probably still need your proposal for other packages, for example, sentry. Let me know when you require a review for this 🙌

@BetoFandino
Copy link
Contributor Author

Hi @MaferMazu, super thank you very much but then I ask you two little things:
- Can we assume that they are going to apply that for sentry and therefore we close this pr and wait for you? or do you need our help to push that?
- I'm not sure if you removed it but that PR you put, if you made social-core mandatory then it would be good to remove this too https://github.com/eduNEXT/eox-core/blob/master/setup.py#L76 ?

@MaferMazu
Copy link
Contributor

MaferMazu commented Nov 28, 2023

You already have this PR @BetoFandino; we can continue the process of adding the try-except to sentry imports in your PR, and in the palm support PR, handle the social_core imports. About the TPA requirements, thanks for that, I didn't notice, I will remove it in #254

@MaferMazu MaferMazu self-assigned this Nov 28, 2023
@BetoFandino
Copy link
Contributor Author

Ok @MaferMazu, then I will make the pertinent changes in this PR. Thank you very much!!! 😃

@MaferMazu
Copy link
Contributor

@BetoFandino, do you still want to merge this?

@BetoFandino
Copy link
Contributor Author

@MaferMazu if possible yes please

@MaferMazu
Copy link
Contributor

@BetoFandino, can you fix the issues, please?

@bra-i-am
Copy link
Contributor

bra-i-am commented Mar 4, 2024

hi! I hope you're well.

@BetoFandino, just a friendly reminder about this PR so we can continue the merging process, thank you c:

@MaferMazu MaferMazu added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Mar 18, 2024
@MaferMazu
Copy link
Contributor

Thanks for updating this, @BetoFandino. I only have a question: Why do we need the file coursekey_h_v1? In my understanding, that file is to support the Open edX Release Hawthorn, and right now, we have a coursekey_m_v1 that supports Maple and all the next releases.

The rest looks good to me. When we figure out the Hawthorn backend, we will be good to merge.

@BetoFandino
Copy link
Contributor Author

Hi @MaferMazu , yes you are right coursekey_h_v1 is no longer needed, I removed it and updated the PR.

Copy link
Contributor

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

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

Looks good to me

@BetoFandino BetoFandino force-pushed the master branch 4 times, most recently from ab47a0f to 99db7d1 Compare May 17, 2024 17:05
@MaferMazu MaferMazu merged commit 50cb82e into eduNEXT:master May 17, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants