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

fix: fix replication of multiple projects with numeric names #21474

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cgirard-mir
Copy link

@cgirard-mir cgirard-mir commented Jan 31, 2025

Thank you for contributing to Harbor!

Comprehensive Summary of your change

The original fix to #19027 contained in #19090 worked well for replicating a single project whose name was numeric, but did not resolve the issue when multiple numeric project names were provided.

The issue was in the construction of the API call. The query parameter was previously being constructed as name={'123 456 789'}. With the addition of some debug logging (also included in this PR), I noticed that the server was parsing this as '123, 456, 789'. Note that the single quotes were being associated with the first and last values of the list, so these values were forced into parsing as string values. However, the middle value contains only numbers and so was parsed as such. This doesn't cause a problem when only 1 or 2 values are being passed, but can show itself once there are 3 or more values.

The solution is to surround each project name in quotes, name={'123' '456' '789'}, so they are all handled as strings by the server.

Issue being fixed

Further fixes #19027.

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

@cgirard-mir cgirard-mir force-pushed the 19027-fix-replication-numeric-project-names branch from e841434 to dd06536 Compare January 31, 2025 21:21
@cgirard-mir cgirard-mir marked this pull request as ready for review February 3, 2025 01:39
@cgirard-mir cgirard-mir requested a review from a team as a code owner February 3, 2025 01:39
@Vad1mo Vad1mo added the release-note/update Update or Fix label Feb 3, 2025
Copy link

codecov bot commented Feb 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.24%. Comparing base (c8c11b4) to head (dd06536).
Report is 378 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #21474      +/-   ##
==========================================
+ Coverage   45.36%   46.24%   +0.87%     
==========================================
  Files         244      247       +3     
  Lines       13333    13883     +550     
  Branches     2719     2875     +156     
==========================================
+ Hits         6049     6420     +371     
- Misses       6983     7126     +143     
- Partials      301      337      +36     
Flag Coverage Δ
unittests 46.24% <ø> (+0.87%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 491 files with indirect coverage changes

@Vad1mo
Copy link
Member

Vad1mo commented Feb 3, 2025

nice catch!

@cgirard-mir cgirard-mir force-pushed the 19027-fix-replication-numeric-project-names branch from 384c66e to dd06536 Compare February 5, 2025 17:39
This keeps the server from parsing all-numeric project names as integer
values which it does not like.

Signed-off-by: Chris Girard <[email protected]>
@cgirard-mir cgirard-mir force-pushed the 19027-fix-replication-numeric-project-names branch from dd06536 to 1f6f427 Compare February 5, 2025 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/update Update or Fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When the project name is a number, the replication error
5 participants