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(service): improve formatting for migrationscheck response #2122

Merged
merged 23 commits into from Nov 15, 2021

Conversation

Panaetius
Copy link
Member

@Panaetius Panaetius commented Jun 2, 2021

closes #2048
/deploy extra-values=core.debug=true

@Panaetius Panaetius requested a review from a team as a code owner June 2, 2021 10:03
@Panaetius Panaetius force-pushed the 2048-finegrained-migration-information branch from f33089c to 6c27f2d Compare June 2, 2021 10:04
@Panaetius Panaetius temporarily deployed to renku-ci-rp-2122 June 3, 2021 13:27 Inactive
@RenkuBot
Copy link
Contributor

RenkuBot commented Jun 3, 2021

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

@Panaetius Panaetius force-pushed the 2048-finegrained-migration-information branch from 6c27f2d to e3acfee Compare June 4, 2021 07:45
@Panaetius Panaetius temporarily deployed to renku-ci-rp-2122 June 4, 2021 07:46 Inactive
@Panaetius Panaetius force-pushed the 2048-finegrained-migration-information branch from e3acfee to d3172a2 Compare June 4, 2021 15:18
@Panaetius Panaetius temporarily deployed to renku-ci-rp-2122 June 4, 2021 15:19 Inactive
@Panaetius Panaetius force-pushed the 2048-finegrained-migration-information branch from d3172a2 to 3e9088e Compare June 4, 2021 15:54
@Panaetius Panaetius force-pushed the 2048-finegrained-migration-information branch from 3e9088e to 3c2b73f Compare June 4, 2021 16:05
@Panaetius Panaetius temporarily deployed to renku-ci-rp-2122 June 4, 2021 16:06 Inactive
@Panaetius Panaetius force-pushed the 2048-finegrained-migration-information branch from 3c2b73f to 3fb27b5 Compare June 7, 2021 08:23
@Panaetius Panaetius temporarily deployed to renku-ci-rp-2122 June 7, 2021 08:24 Inactive
@Panaetius Panaetius force-pushed the 2048-finegrained-migration-information branch from 3fb27b5 to 99b1f3d Compare June 8, 2021 07:10
@Panaetius Panaetius temporarily deployed to renku-ci-rp-2122 June 8, 2021 07:10 Inactive
@Panaetius Panaetius temporarily deployed to renku-ci-rp-2122 June 8, 2021 13:35 Inactive
@Panaetius Panaetius changed the title feat(service): improve formatting or migrationscheck response feat(service): improve formatting for migrationscheck response Jun 9, 2021
@Panaetius Panaetius temporarily deployed to renku-ci-rp-2122 June 9, 2021 06:30 Inactive
@Panaetius Panaetius temporarily deployed to renku-ci-rp-2122 June 10, 2021 06:56 Inactive
@rokroskar rokroskar temporarily deployed to renku-ci-rp-2122 June 14, 2021 09:55 Inactive
@Panaetius Panaetius force-pushed the 2048-finegrained-migration-information branch from c99e198 to 795e8a3 Compare September 16, 2021 14:32
@Panaetius Panaetius temporarily deployed to renku-ci-rp-2122 September 16, 2021 14:32 Inactive
@Panaetius Panaetius force-pushed the 2048-finegrained-migration-information branch from e06a924 to f99fb98 Compare November 1, 2021 13:22
Copy link
Contributor

@m-alisafaee m-alisafaee left a comment

Choose a reason for hiding this comment

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

Thanks Ralf! This looks pretty good.

renku/core/commands/migrate.py Outdated Show resolved Hide resolved
renku/service/serializers/cache.py Outdated Show resolved Hide resolved
renku/service/serializers/cache.py Outdated Show resolved Hide resolved
renku/core/commands/migrate.py Outdated Show resolved Hide resolved
@m-alisafaee
Copy link
Contributor

Isn't core_renku_version the same as dockerfile_renku_status.latest_renku_version? Is there a reason to have redundant info here? Also project_renku_version comes from the commit agent which should be newer than dockerfile_renku_status.dockerfile_renku_version; do we need both of them here?

@m-alisafaee
Copy link
Contributor

API version is currently done like /v0.9/ but according to the RFC it should be like /0.9/.

@Panaetius
Copy link
Member Author

Isn't core_renku_version the same as dockerfile_renku_status.latest_renku_version? Is there a reason to have redundant info here? Also project_renku_version comes from the commit agent which should be newer than dockerfile_renku_status.dockerfile_renku_version; do we need both of them here?

Yes it is duplicate, UI requested core_renku_version and dockerfile_renku_status.latest_renku_version like this. I don't think it matters having it duplicated and if this makes implementing logic on there side easier, why not.

dockerfile_renku_status.dockerfile_renku_version is relevant for updating the Dockerfile (Can also be None). project_renku_version is irrelevant imo, maybe nice to have to present to a user. But if a user manually modifies Dockerfile, they could be disparate (And the project probably broken). I think the UI just uses project_renku_version to say "Updating from 0.16.2 to 1.0.0" even though what is actually done is updating from 8 to 9, but users don't know what "8" or "9" means.

From our point of view it doesn't matter but I think it's useful for the UI to show to users (to hide some complexity).

@Panaetius Panaetius merged commit 2812659 into master Nov 15, 2021
@Panaetius Panaetius deleted the 2048-finegrained-migration-information branch November 15, 2021 13:43
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.

Return more finegrained Dockerfile update information in service
5 participants