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

UI Changes for Configuring Proxy-Cache Speed Limit #20862

Closed
wants to merge 8 commits into from

Conversation

kunal-511
Copy link
Contributor

@kunal-511 kunal-511 commented Aug 20, 2024

Thank you for contributing to Harbor!

proxy-cache bandwidth limit UI change

Comprehensive Summary of your change

This pull request addresses the UI changes required for configuring the proxy-cache speed limit, as outlined in Issue #20589. The changes include the addition of a bandwidth field for setting the speed limit in both the proxy-cache creation page and the project configuration page

Issue being fixed

Fixes #20811

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.

@kunal-511 kunal-511 changed the title proxy-cache bandwidth limit UI change UI Changes for Configuring Proxy-Cache Speed Limit Aug 20, 2024
@kunal-511 kunal-511 marked this pull request as ready for review August 20, 2024 15:18
@kunal-511 kunal-511 requested a review from a team as a code owner August 20, 2024 15:18
@Vad1mo Vad1mo added the release-note/update Update or Fix label Aug 20, 2024
@@ -246,6 +246,7 @@
"TOGGLED_SUCCESS": "Toggled project successfully.",
"FAILED_TO_DELETE_PROJECT": "Project contains repositories or replication rules or helm-charts cannot be deleted.",
"INLINE_HELP_PUBLIC": "When a project is set to public, anyone has read permission to the repositories under this project, and the user does not need to run \"docker login\" before pulling images under this project.",
"PROXY_CACHE_BANDWIDTH":"Set the maximum network bandwidth to pull image from upstream for proxy-cache. For unlimited bandwidth, please enter -1. ",
Copy link
Member

Choose a reason for hiding this comment

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

this string needs to be added to all language files. (Put it in the english version), for later translation.

Its than easier to pick it up by others and translate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this line to all the language files

@kunal-511 kunal-511 requested a review from Vad1mo August 20, 2024 17:42
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.02%. Comparing base (c8c11b4) to head (269e527).
Report is 285 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main   #20862       +/-   ##
===========================================
+ Coverage   45.36%   69.02%   +23.65%     
===========================================
  Files         244      800      +556     
  Lines       13333   100469    +87136     
  Branches     2719        0     -2719     
===========================================
+ Hits         6049    69352    +63303     
- Misses       6983    27320    +20337     
- Partials      301     3797     +3496     
Flag Coverage Δ
unittests 69.02% <ø> (+23.65%) ⬆️

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

see 1040 files with indirect coverage changes

@stonezdj
Copy link
Contributor

stonezdj commented Aug 30, 2024

Verified the UI, the payload is wrong:

{"project_name":"dockerhub5","metadata":{"public":"true"},"storage_limit":-1,"registry_id":1}

It should be in this format

{"project_name":"dockerhub5","metadata":{"public":"true", "storage_limit":-1},"registry_id":1}

@stonezdj
Copy link
Contributor

Screenshot 2024-08-30 at 15 00 49
The color in the warning message is different from the existing color.

@stonezdj
Copy link
Contributor

stonezdj commented Sep 2, 2024

@kunal-511 could please refer to this PR stonezdj@e39f73e to update your code.

@kunal-511
Copy link
Contributor Author

@stonezdj Updated my PR

@chlins
Copy link
Member

chlins commented Sep 3, 2024

@kunal-511 Please resolve the code conflicts.

@kunal-511
Copy link
Contributor Author

@kunal-511 Please resolve the code conflicts.

Resolved the code conflicts

@kunal-511
Copy link
Contributor Author

@kunal-511 could please refer to this PR stonezdj@e39f73e to update your code.

need to update other language json files ??

@stonezdj
Copy link
Contributor

stonezdj commented Sep 3, 2024

@stonezdj Updated my PR

Please update the language json file to add this line.

        "BANDWIDTH": "Bandwidth",
        "SPEED_LIMIT_TIP": "Please enter -1 or an integer  greater than 0. ",

@kunal-511
Copy link
Contributor Author

@stonezdj Updated my PR

Please update the language json file to add this line.

        "BANDWIDTH": "Bandwidth",
        "SPEED_LIMIT_TIP": "Please enter -1 or an integer  greater than 0. ",

Need to add this to all language json files?

@stonezdj
Copy link
Contributor

stonezdj commented Sep 4, 2024

@stonezdj Updated my PR

Please update the language json file to add this line.

        "BANDWIDTH": "Bandwidth",
        "SPEED_LIMIT_TIP": "Please enter -1 or an integer  greater than 0. ",

Need to add this to all language json files?

Yes

@kunal-511
Copy link
Contributor Author

@stonezdj Updated my PR

Please update the language json file to add this line.

        "BANDWIDTH": "Bandwidth",
        "SPEED_LIMIT_TIP": "Please enter -1 or an integer  greater than 0. ",

Need to add this to all language json files?

Yes

Added

@@ -1,6 +1,9 @@
#severity-blk div {
display: inline-block;
}
.mr-10 {
Copy link
Contributor

Choose a reason for hiding this comment

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

The current UI change is only limit to create project, why change the CSS of the project policy config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current UI change is only limit to create project, why change the CSS of the project policy config?

It was initially told by @zyyw that we need to make changes to both the create project and also to the project policy-config page

@stonezdj
Copy link
Contributor

stonezdj commented Sep 9, 2024

Please squash all commit into one @kunal-511

@kunal-511
Copy link
Contributor Author

Please squash all commit into one @kunal-511

Yes I will do this

(value <= 0 && value !== -1)
) {
this.bandwidthError =
'Please enter -1 or an integer greater than 0.';
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove extra white space between word 'integer' and 'greater'

"OF": "共計",
"COUNT_QUOTA": "數量配額",
"STORAGE_QUOTA": "儲存配額限制",
"COUNT_QUOTA_TIP": "請輸入介於 '1' 和 '100,000,000' 之間的整數,'-1' 表示無限制。",
"STORAGE_QUOTA_TIP": "儲存配額上限只接受整數,上限為 '1024TB'。輸入 '-1' 表示無限配額。",
"QUOTA_UNLIMIT_TIP": "專案所能使用的最大邏輯空間。輸入 '-1' 表示無限制配額。",
"BANDWIDTH": "Bandwidth",
"SPEED_LIMIT_TIP": "Please enter -1 or an integer greater than 0. ",
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the extra indent

>
<div class="flex">
<input
class="clr-input width-164 mr-10"
Copy link
Contributor

Choose a reason for hiding this comment

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

why does the [(ngModel)] not configured for the input element?

<div class="clr-select-wrapper mr-10">
<select name="" id="">
<option value="kbps" selected>kbps</option>
<option value="Mbps">Mbps</option>
Copy link
Contributor

Choose a reason for hiding this comment

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

[(ngModel)] is not configured for select. why?

@stonezdj
Copy link
Contributor

@kunal-511 could you please fix the review comment issue before the FC date(Sept 22)?

@kunal-511
Copy link
Contributor Author

@stonezdj I have created a new PR with single commit and also addressed above issues. Please have a look #20946

@kunal-511 kunal-511 closed this Oct 2, 2024
@kunal-511 kunal-511 deleted the feat-#20811 branch October 2, 2024 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proxy-cache bandwidth limit UI change
7 participants