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

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

Merged
merged 60 commits into from
Jan 31, 2024

Conversation

QuocDuong1306
Copy link

  • Migrate to v16
  • Change the dependency from storage_backend to fs_storage

simahawk and others added 30 commits October 10, 2023 15:10
Ack files should be treated as incoming records.
Receive files is not handled yet by the framework
and will be added soon.
* A cron job check related storage on edi backends to create the
  needed exchange files.
* Add basic receive component.
…filenames from storage.

Also extend help in `exchange_filename_pattern` field to clarify usage.
edi_storage_oca/components/base.py Outdated Show resolved Hide resolved
edi_storage_oca/components/check.py Outdated Show resolved Hide resolved
edi_storage_oca/components/listener.py Show resolved Hide resolved
edi_storage_oca/components/listener.py Outdated Show resolved Hide resolved
@@ -39,7 +39,7 @@ def test_export_file_already_done(self):
self.record.edi_exchange_state = "output_sent"
mocked_paths = {self._file_fullpath("done"): self.fakepath}
# TODO: test send only w/out cron (make sure check works)
self._test_run_cron(mocked_paths)
self._test_run_cron(mocked_paths, skip_sent=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we have to add this change?

Copy link
Author

Choose a reason for hiding this comment

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

Because in tests that have some case with the status output_sent so we use the flag skip_sent to ignore another case:

  • skip_sent = True --> ignore records that were already sent
  • skip_sent = False ---> send it

Copy link
Contributor

Choose a reason for hiding this comment

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

I know what it does 😉
I mean: why was not needed before and it is?

bits.append(r"\." + exchange_type.exchange_file_ext)
pattern = "".join(bits)
# TODO: clean this up, .find_files is deprecated in fs_storage
full_paths = self.storage_id.find_files(pattern, full_input_dir_pending)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @QuocDuong1306 and @simahawk

I am testing this PR and the method find_files (https://github.com/OCA/storage/blob/1a4d80faaab0ddf6dfc80d867472dd6ce4777c5f/fs_storage/models/fs_storage.py#L373) is not returning the full path from the root for me. Maybe you know what I may be missing.

I have an FTP where the home folder contains /import folder to import files. When the call to the method is done like self.fs.ls(relative_path, detail=False) being relative_path "/import", I just get the filename of the file inside the folder.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll have a look soon

Copy link
Contributor

Choose a reason for hiding this comment

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

@JordiMForgeFlow are you using directory_path? If yes, you might have inconsistent results. See OCA/storage#311

Copy link
Contributor

Choose a reason for hiding this comment

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

@simahawk I am not using directory_path. Nevertheless, I have been testing and I have been able to get it working fine by assuming that find_files returns only the filename, not the full path.

The problem is mainly that, if for example I have a file test_file.xml in a folder called import, the find_files method returns test_file.xml. Then, when the line return [p[pending_path_len:].strip("/") for p in full_paths] is executed, the filename stored in the exchange record is left as ile.xml (stripping the first characters as it assumes that it is actually the full path)

# otherwise is impossible to retrieve input files and acks
# (the date will never match)
# TODO: clean this up, .get is deprecated in fs_storage
return self.storage.get(path.as_posix(), binary=binary)
Copy link
Contributor

Choose a reason for hiding this comment

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

@simahawk @QuocDuong1306 another issue that I am seeing is when sending a file. In the send component, a check is done before sending to make sure a file is not sent twice. The check ends up in this line, doing a storage.get.

I suppose it has to do with the new fs storage, but doing the get for a file that does not exist raises a different error than FileNotFoundError. The error is:

ValueError: <class 'ftplib.error_perm'>: "550 Can't open *****: No such file or directory" (being ***** the filepath being accessed)

Copy link
Author

Choose a reason for hiding this comment

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

I have created a PR at here: fsspec/filesystem_spec#1494

@john-herholz-dt
Copy link
Contributor

What is the stage of this PR?

@JordiMForgeFlow
Copy link
Contributor

Hi @john-herholz-dt

It works fine except for the issue I mentioned #26 (comment). 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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

@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 😉)

@john-herholz-dt
Copy link
Contributor

Super fine!

@JordiMForgeFlow
Copy link
Contributor

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

@simahawk
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

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 OCA-git-bot merged commit 5256e6b into OCA:16.0 Jan 31, 2024
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@etobella
Copy link
Member

@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
Copy link
Contributor

@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 😉

@nilshamerlinck
Copy link
Contributor

@simahawk proposal of simple migration script pushed here : #95
note that sensitive parts of conf (host, port, credentials) is coming from the environment and will still need manual adaptation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet