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

Base._ls(): may always return an empty list #178

Merged
merged 4 commits into from
Feb 5, 2024

Conversation

frankenjoe
Copy link
Collaborator

@frankenjoe frankenjoe commented Jan 31, 2024

So far the docstring of Base._ls() was saying:

        If ``path`` is ``'/'`` and no files exist on the repository,
        an empty list should be returned
        Otherwise,
        if ``path`` does not exist or no files are found under ``path``,
        an error should be raised.

This might be overlooked by a developer, so here we simplify _ls() by not demanding to raise an error message or treat / as a special case. Instead it is sufficient to return an empty list, even in the case that the path does not exist. The docstring now says:

        If ``path`` does not exist
        an empty list can be returned.

To avoid a breaking change, raising a file-not-found-error is now handled by Base, in the same way we already do it for the case that path is not a sub-path.

@frankenjoe frankenjoe changed the title Base.ls(): always return Base.ls(): return empty list if path does not exist Jan 31, 2024
@frankenjoe frankenjoe changed the title Base.ls(): return empty list if path does not exist Base._ls(): return empty list if path does not exist Jan 31, 2024
@frankenjoe frankenjoe changed the title Base._ls(): return empty list if path does not exist Base._ls(): may always return an empty list Jan 31, 2024
@frankenjoe
Copy link
Collaborator Author

@hagenw locally tests with Artifactory are passing, but here it seems that it can no longer access the server. Any idea?

@frankenjoe frankenjoe requested a review from hagenw January 31, 2024 11:08
@hagenw
Copy link
Member

hagenw commented Feb 1, 2024

There is a different when running locally or here in the CI as the CI uses the audeering-unittest user, whereas we locally use our user names. But all of those users have exactly the same rights in the Artifactory server.

The error message of the failing test was:

E           dohq_artifactory.exception.ArtifactoryException: This request is blocked due to recurrent login failures, please try again in 7 seconds

My suspicision is that every time we try to access a file that does not exist it will be counted as a login failure. And as we have several CI jobs running in parallel, it might be that we reach the blocking threshold too early. To me it does not sound like a good design on the Artifactory side that a user gets block even she/he has a correct username/password login.

I guess we need to check if there is a setting on the Artifactory server to change the behavior. If not, we might have to reduce the number of pipelines. Or group tests for error messages on missing data in a separate file and run this file only in a single CI job.

@hagenw
Copy link
Member

hagenw commented Feb 1, 2024

Maybe there is also an error with the stored secret on Github. I will try to replace it with a new one.

@frankenjoe
Copy link
Collaborator Author

But isn't it strange we only encounter this problem now?

@frankenjoe
Copy link
Collaborator Author

Maybe there is also an error with the stored secret on Github. I will try to replace it with a new one.

Ok, thanks.

@hagenw
Copy link
Member

hagenw commented Feb 1, 2024

But isn't it strange we only encounter this problem now?

I also encountered the problem when I tried to add tests for non-existing and private repositories in audeering/audb#339

@hagenw
Copy link
Member

hagenw commented Feb 1, 2024

I replaced the secret on Github and re-started the CI jobs, but unfortunately the error stays the same.

@hagenw
Copy link
Member

hagenw commented Feb 1, 2024

It seems to be common that secrets are replaced by "***" in the output of CI jobs. Here is the output of an older passing one:

image

@hagenw
Copy link
Member

hagenw commented Feb 1, 2024

I tested a few more things and the username and API key seem still to be correctly set, but the error remains in the end:

E           dohq_artifactory.exception.ArtifactoryException: This request is blocked due to recurrent login failures, please try again in 13 seconds

Maybe we wait until next week to see if the error remains?

@frankenjoe
Copy link
Collaborator Author

Maybe we wait until next week to see if the error remains?

ok

Copy link

codecov bot commented Feb 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (65c6844) 100.0% compared to head (fa10a86) 100.0%.

❗ Current head fa10a86 differs from pull request most recent head 22ab9e4. Consider uploading reports for the commit 22ab9e4 to get more accurate results

Additional details and impacted files
Files Coverage Δ
audbackend/core/backend/artifactory.py 100.0% <100.0%> (ø)
audbackend/core/backend/base.py 100.0% <100.0%> (ø)
audbackend/core/backend/filesystem.py 100.0% <100.0%> (ø)

@hagenw
Copy link
Member

hagenw commented Feb 5, 2024

As usual, waiting a few days helps to solve login issues.

Co-authored-by: Hagen Wierstorf <hwierstorf@audeering.com>
@hagenw hagenw merged commit 4647863 into main Feb 5, 2024
7 checks passed
@hagenw hagenw deleted the ls-implementation-always-returns branch February 5, 2024 08:33
hagenw added a commit that referenced this pull request May 3, 2024
* Base.ls(): always return

* DOC: update

* DOC: update comment

* Update audbackend/core/backend/base.py

Co-authored-by: Hagen Wierstorf <hwierstorf@audeering.com>

---------

Co-authored-by: Hagen Wierstorf <hwierstorf@audeering.com>
hagenw added a commit that referenced this pull request May 3, 2024
* Base.ls(): always return

* DOC: update

* DOC: update comment

* Update audbackend/core/backend/base.py

Co-authored-by: Hagen Wierstorf <hwierstorf@audeering.com>

---------

Co-authored-by: Hagen Wierstorf <hwierstorf@audeering.com>
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.

2 participants