search

OCA / edi-framework

GNU Affero General Public License v3.0
6 stars 29 forks source link

[16.0][MIG] edi_storage_oca: Migration to 16.0 #26

Closed QuocDuong1306 closed 7 months ago

QuocDuong1306 commented 11 months ago
john-herholz-dt commented 7 months ago

What is the stage of this PR?

JordiMForgeFlow commented 7 months ago

Hi @john-herholz-dt

It works fine except for the issue I mentioned https://github.com/OCA/edi-framework/pull/26#discussion_r1429819870. I have been using a custom branch where I simply fixed what I commented and it has been working fine for some weeks already.

john-herholz-dt commented 7 months ago

@JordiMForgeFlow , thanks for clarifying.

Why not add this fix to this branch? At the moment the code coverage target isn't reached.

Will @simahawk merge it anyway or do we need to get the coverage? Are you then continuing here or do you need help?

I need this module actually for 17.0 so I am waiting. =) But thanks for the contribution so far, great work!

JordiMForgeFlow commented 7 months ago

@john-herholz-dt

The fix I added is basically changing return [p[pending_path_len:].strip("/") for p in full_paths] for return [p.strip("/") for p in full_paths], as in my case (SFTP connection) I was not getting full paths. However, I am not sure if it has to do with my specific case or if it needs to be like this always.

simahawk commented 7 months ago

@john-herholz-dt @JordiMForgeFlow @QuocDuong1306 If it's fine for you all I'd merge what we have here as it is and then we iterate again for fixes (whoever has the chance to do it 1st :wink:)

john-herholz-dt commented 7 months ago

Super fine!

JordiMForgeFlow commented 7 months ago

@simahawk LGTM! I will open a PR with the fix regarding the paths.

simahawk commented 7 months ago

/ocabot merge nobump

OCA-git-bot commented 7 months ago

Hey, thanks for contributing! Proceeding to merge this for you. Prepared branch 16.0-ocabot-merge-pr-26-by-simahawk-bump-nobump, awaiting test results.

OCA-git-bot commented 7 months ago

Congratulations, your PR was merged at 7abe3a4ffab79ad3e457a96a234c3b8b462a215b. Thanks a lot for contributing to OCA. ❤️

etobella commented 2 months ago

@QuocDuong1306 why did you change storage_backend to fs_storage if there is no migration scripts there? If we migrate from 15 to 16, all the backends that are related to a storage will fail...

Actually, storage_backend module already exists on 16 and this module should replace the original module. But no migration scrips have been created besides a comment on an issue.

@simahawk @JordiMForgeFlow @lmignon

simahawk commented 2 months ago

@nilshamerlinck @QuocDuong1306 could you please have a look and evaluate how to get a script to migrate? I would assume that storage_backend will still be installed in the old db and that we simply have to create the matching fs.storage records w/ their conf leaving the old storage.backend records untouched. The conversion part can be a util to be added to fs_storage but let's start small for now :wink:

nilshamerlinck commented 2 months ago

@simahawk proposal of simple migration script pushed here : https://github.com/OCA/edi-framework/pull/95 note that sensitive parts of conf (host, port, credentials) is coming from the environment and will still need manual adaptation