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

fix(service): disable migration check optimization for anonymous sessions #2541

Merged
merged 5 commits into from Jan 14, 2022

Conversation

m-alisafaee
Copy link
Contributor

@m-alisafaee m-alisafaee commented Dec 22, 2021

Description

Disable optimized /cache.migrations_check for now.

/deploy renku=1.0-next renku-graph=cli-v1 renku-ui=1507b-migrations-giturl #notest

@Panaetius
Copy link
Member

Maybe for anonymous users we should just go the slow route instead of disabling it (execute_op instead of _fast_op_without_cache)

lorenzo-cavazzi added a commit to SwissDataScienceCenter/renku-ui that referenced this pull request Jan 6, 2022
- Allow anonymous users to invoke renku-core APIs (where possible)
- Switch to using git_url + branch for migrations instead of project_id

BREAKING CHANGE: requires SwissDataScienceCenter/renku-python#2541

re #1507
lorenzo-cavazzi added a commit to SwissDataScienceCenter/renku-ui that referenced this pull request Jan 7, 2022
- Allow anonymous users to invoke renku-core APIs (where possible)
- Switch to using git_url + branch for migrations instead of project_id

BREAKING CHANGE: requires SwissDataScienceCenter/renku-python#2541

re #1507
lorenzo-cavazzi added a commit to SwissDataScienceCenter/renku-ui that referenced this pull request Jan 7, 2022
- Allow anonymous users to invoke renku-core APIs (where possible)
- Switch to using git_url + branch for migrations instead of project_id

BREAKING CHANGE: requires SwissDataScienceCenter/renku-python#2541

re #1507
@m-alisafaee m-alisafaee force-pushed the 0000-disbale-check-migration-optimization branch from 9b1dc4e to 25283b2 Compare January 10, 2022 13:02
@m-alisafaee m-alisafaee changed the title chore(service): disable migration check optimization fix(service): disable migration check optimization for anonymous sessions Jan 10, 2022
@m-alisafaee m-alisafaee marked this pull request as ready for review January 10, 2022 16:23
@m-alisafaee m-alisafaee requested a review from a team as a code owner January 10, 2022 16:23
Panaetius
Panaetius previously approved these changes Jan 10, 2022
Copy link
Member

@Panaetius Panaetius left a comment

Choose a reason for hiding this comment

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

Thank you!

@lorenzo-cavazzi
Copy link
Member

Hold on, we "accidentally" tried this on SwissDataScienceCenter/renku-ui#1630 (we re-deployed after the last changes) and we noticed a problem with logged users
https://sentry.dev.renku.ch/organizations/swiss-datascience-center/issues/7793

Atm we switched to another branch taken from the previous commit, so it will work if you try on the UI CI deployment.
You could deploy a local version for testing with:

/deploy renku=1.0-next renku-graph=cli-v1 renku-ui=1507b-migrations-giturl

@Panaetius
Copy link
Member

That's weird. So the token sent in the JWT with that user does not have access to that repo through gitlab API, even though the repo is owned by that user.

@lorenzo-cavazzi lorenzo-cavazzi temporarily deployed to renku-ci-rp-2541 January 12, 2022 15:45 Inactive
@RenkuBot
Copy link
Contributor

You can access the deployment of this PR at https://renku-ci-rp-2541.dev.renku.ch

@lorenzo-cavazzi
Copy link
Member

That's weird. So the token sent in the JWT with that user does not have access to that repo through gitlab API, even though the repo is owned by that user.

That seems to be an issue also on public projects, where an invalid token shouldn't be a problem .

Anyway, I modified the initial message to trigger a CI deployment.
You can verify the error here by trying to access as non logged user (it works) and then as logged user (currently it doesn't work, and the the GET /cache.migrations_check endpoint return the following error message {"error": {"code": -32603, "reason": "internal error"}})
https://renku-ci-rp-2541.dev.renku.ch/projects/lorenzo.cavazzi.tech/project-with-core-1-0-1/overview/status

@m-alisafaee m-alisafaee deployed to renku-ci-rp-2541 January 14, 2022 13:32 Active
lorenzo-cavazzi added a commit to SwissDataScienceCenter/renku-ui that referenced this pull request Jan 14, 2022
- Allow anonymous users to invoke renku-core APIs (where possible)
- Switch to using git_url + branch for migrations instead of project_id

BREAKING CHANGE: requires SwissDataScienceCenter/renku-python#2541

re #1507
Copy link
Member

@Panaetius Panaetius left a comment

Choose a reason for hiding this comment

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

Thank you!

@m-alisafaee m-alisafaee merged commit f2f573f into develop Jan 14, 2022
@m-alisafaee m-alisafaee deleted the 0000-disbale-check-migration-optimization branch January 14, 2022 15:25
@lorenzo-cavazzi lorenzo-cavazzi restored the 0000-disbale-check-migration-optimization branch January 14, 2022 15:28
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.

None yet

4 participants