add apps launcher handling#1356
Conversation
|
You can access the deployment of this PR at https://renku-ci-ds-1356.dev.renku.ch |
Coverage Report for CI Build 28371791278Warning No base build found for commit Coverage: 86.137%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
wesjdj
force-pushed
the
add-apps-list-patch
branch
from
4661405 to
7d804ae
Compare
wesjdj
force-pushed
the
add-apps-launcher-handling
branch
from
2e13a12 to
ce99c90
Compare
olevski
left a comment
olevski
left a comment
There was a problem hiding this comment.
One small thing. But if you prefer we can address everything in future/follow up PRs.
| project = await self.project_repo.get_project(user, launcher.project_id) | ||
| runtime_state = await self.k8s_client.get_app_deployment_for_project(project, launcher) | ||
| if runtime_state is None: | ||
| return None | ||
| await self.delete_app(user, runtime_state.name) | ||
| return None |
There was a problem hiding this comment.
We should be able to use labels in k8s so that we can retrieve the app based on the launcher id. Or alternatively we can save the application name from k8s into the launcher table. But the launcher table is already getting complicated. So I would go towarrds finding the app with the launcher labels. That way you dont have to find the project or the launcher.
There was a problem hiding this comment.
You can also try to find it based on reconstructing the name from the project and launcher and if that fails then fall back to the labels.
| if project_patch.visibility == Visibility.PRIVATE and await self.session_repo.project_has_app_launchers( | ||
| user, project_id | ||
| ): | ||
| raise errors.ValidationError( | ||
| message="This project cannot be made private because it has one or more app launchers." | ||
| ) |
There was a problem hiding this comment.
comment(non-blocking): This is fine. Just not sure what the desired behaviour is. but if we need to change the behaviour I propose we do it in a follow up PR. I asked for confirmation in slack here: https://swiss-data-science.slack.com/archives/C0B1W8RBQBX/p1782737951990709
There was a problem hiding this comment.
Got confirmation that the desired behaviour is to not require the user to delete the app. But the backend should turn off the app and then prevent the app from being restarted as long as the project is private. But lets come back to this in a followup PR.
There was a problem hiding this comment.
Or if you really want to address it here - then go ahead.
…n private projects
4 participants
/deploy renku=add-apps-beta extra-values=apps.enabled=true