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

global: make sentry-sdk optional #145

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

slint
Copy link
Member

@slint slint commented Dec 2, 2024

  • Import-detects sentry_sdk so that we can remove the hard dependency.

* Import-detects `sentry_sdk` so that we can remove the hard dependency.
@slint
Copy link
Member Author

slint commented Dec 2, 2024

@zzacharo @ntarocco, we're considering with @utnapischtim, to release this as a patch or minor, since adding the dependency in the first place might not have been correct (since we're enforcing an infra dependency on the entirety of Invenio(RDM)). WDYT?

@ntarocco
Copy link
Contributor

ntarocco commented Dec 2, 2024

@zzacharo @ntarocco, we're considering with @utnapischtim, to release this as a patch or minor, since adding the dependency in the first place might not have been correct (since we're enforcing an infra dependency on the entirety of Invenio(RDM)). WDYT?

Agree, we should aware add a line in the InvenioRDM release notes (v12 included?) that Sentry was removed, and it should be added manually in each instance.

@ntarocco
Copy link
Contributor

ntarocco commented Dec 2, 2024

Looks like we need to fix this too

@utnapischtim
Copy link
Contributor

Agree, we should aware add a line in the InvenioRDM release notes (v12 included?) that Sentry was removed, and it should be added manually in each instance.

thats for me a reason why a major release would be the better option. v12 should not be touched with such changes.

@slint
Copy link
Member Author

slint commented Dec 2, 2024

I'm arguing against a major since I think Sentry is mainly something CERN instances have been using historically (though probably asking on Discord might be the better way to determine this).

Otherwise, we also need to do major bump in the following packages (and their dependencies as well):

invenio-rest v1.4.2
├── invenio-accounts v5.1.7 [requires: invenio-rest >=1.2.4, <2.0.0]
├── invenio-app-rdm v13.0.0b1.dev23 [requires: invenio-rest >=1.3.0, <2.0.0]
├── invenio-oaiserver v2.2.3 [requires: invenio-rest >=1.2.4, <2.0.0]
├── invenio-pages v4.1.2 [requires: invenio-rest >=1.2.0, <2.0.0]
└── invenio-records-rest v2.4.1 [requires: invenio-rest >=1.2.4, <2.0.0]

@utnapischtim
Copy link
Contributor

i could include that into my major release chain. i will start today or tomorrow with that experience

@slint
Copy link
Member Author

slint commented Dec 2, 2024

We decided on a minor to minimize overall impact on v12 and v13, and avoid making the current Flask v3 efforts harder than needed.

@slint slint merged commit 5246a0e into inveniosoftware:master Dec 2, 2024
2 checks passed
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.

4 participants