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.

Sign up for GitHub

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

Jump to bottom

Feat: add acr ee support #19658

Merged
merged 8 commits into from
Oct 11, 2024
Merged

Feat: add acr ee support #19658

merged 8 commits into from
Oct 11, 2024

Conversation

njucjc
Copy link
Contributor

@njucjc njucjc commented Dec 1, 2023 •
edited
Loading

Thank you for contributing to Harbor!

Comprehensive Summary of your change

Feat: add acr ee support

Issue being fixed

Fixes #19659

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.

@njucjc njucjc requested a review from a team as a code owner December 1, 2023 07:53
@njucjc
Copy link
Contributor Author

njucjc commented Dec 1, 2023

@AllForNothing @chlins @MinerYang @ctrlaltdel PTAL

@njucjc njucjc force-pushed the support_acr_ee branch 3 times, most recently from 0a6ec17 to b3c42d6 Compare December 1, 2023 14:39
@codecov Codecov
Copy link

codecov bot commented Dec 4, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 13.60544% with 254 lines in your changes missing coverage. Please review.

Project coverage is 66.08%. Comparing base (c8c11b4) to head (7c1f329).
Report is 291 commits behind head on main.

Files with missing lines Patch % Lines
src/pkg/reg/adapter/aliacr/openapi.go 0.00% 207 Missing ⚠️
src/pkg/reg/adapter/aliacr/adapter.go 51.28% 38 Missing ⚠️
src/pkg/reg/adapter/aliacr/auth.go 0.00% 9 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main   #19658       +/-   ##
===========================================
+ Coverage   45.36%   66.08%   +20.71%     
===========================================
  Files         244     1049      +805     
  Lines       13333   114634   +101301     
  Branches     2719     2856      +137     
===========================================
+ Hits         6049    75751    +69702     
- Misses       6983    34741    +27758     
- Partials      301     4142     +3841
Flag Coverage Δ
unittests 66.08% <13.60%> (+20.71%) ⬆️

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

Files with missing lines Coverage Δ
src/pkg/reg/adapter/aliacr/types.go 0.00% <ø> (ø)
src/pkg/reg/adapter/aliacr/auth.go 23.52% <0.00%> (ø)
src/pkg/reg/adapter/aliacr/adapter.go 46.66% <51.28%> (ø)
src/pkg/reg/adapter/aliacr/openapi.go 0.00% <0.00%> (ø)

... and 1284 files with indirect coverage changes

@wy65701436
Copy link
Contributor

Thanks, @njucjc for your contribution.

I have a couple of basic questions:

  • Is it possible to set up a staging environment of ACR EE for our validation and regression purposes?
  • Also, considering that the replication adapter ships with harbor-core, which requires CVE and bug fixes, would you be responsible for maintaining this adapter?

@wy65701436 wy65701436 added the release-note/new-feature New Harbor Feature label Dec 4, 2023
@njucjc
Copy link
Contributor Author

njucjc commented Dec 4, 2023 •
edited
Loading

Thanks, @njucjc for your contribution.

I have a couple of basic questions:

  • Is it possible to set up a staging environment of ACR EE for our validation and regression purposes?
  • Also, considering that the replication adapter ships with harbor-core, which requires CVE and bug fixes, would you be responsible for maintaining this adapter?

@wy65701436

  • Of course, I can provide an ACR EE instance for regression verification. What is the best way for me to provide this ACR EE instance for you to verify？it has been verified by myself.
  • Sure, It's my honor to maintain this adapter.

By the way, I fix some UTTEST errors

@njucjc njucjc force-pushed the support_acr_ee branch 2 times, most recently from 4942da0 to efd7515 Compare December 4, 2023 09:04
@njucjc
Copy link
Contributor Author

njucjc commented Dec 7, 2023

any comments?

@njucjc njucjc mentioned this pull request Dec 12, 2023
Merged
5 tasks
@njucjc
Copy link
Contributor Author

njucjc commented Dec 12, 2023 •
edited
Loading

Thanks, @njucjc for your contribution.
I have a couple of basic questions:

  • Is it possible to set up a staging environment of ACR EE for our validation and regression purposes?
  • Also, considering that the replication adapter ships with harbor-core, which requires CVE and bug fixes, would you be responsible for maintaining this adapter?

@wy65701436

  • Of course, I can provide an ACR EE instance for regression verification. What is the best way for me to provide this ACR EE instance for you to verify？it has been verified by myself.
  • Sure, It's my honor to maintain this adapter.

By the way, I fix some UTTEST errors

@wy65701436 I send an ACR EE acount for you in email

@github-actions GitHub Actions
Copy link

This PR is being marked stale due to a period of inactivty. If this PR is still relevant, please comment or remove the stale label. Otherwise, this PR will close in 30 days.

@github-actions github-actions bot added the Stale label Feb 11, 2024
@github-actions github-actions bot removed the Stale label Feb 22, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 8, 2024

This PR is being marked stale due to a period of inactivty. If this PR is still relevant, please comment or remove the stale label. Otherwise, this PR will close in 30 days.

@github-actions github-actions bot added the Stale label Jun 8, 2024
@OrlinVasilev
Copy link
Member

just a reminder, as discussed please add test cases :)

@njucjc
Copy link
Contributor Author

njucjc commented Jul 25, 2024

just a reminder, as discussed please add test cases :)

I will add ut as soon as possible

chlins
chlins reviewed Aug 9, 2024
View reviewed changes
src/pkg/reg/adapter/aliacr/adapter.go Outdated

func getRegion(url string) (region string, err error) {
func getURL(url string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

getRegistryURL may be more clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

chlins
chlins approved these changes Aug 12, 2024
View reviewed changes
Copy link
Member

@chlins chlins left a comment

Choose a reason for hiding this comment

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

lgtm

@njucjc
Copy link
Contributor Author

njucjc commented Aug 13, 2024

lgtm

@chlins fix some ut error.

@kofj
Copy link
Contributor

kofj commented Aug 20, 2024

@njucjc I watched the demo recording of the community meeting on July 24, 2024. Can you verify that the personal version of ACR is functioning properly at the same time?

@njucjc
Copy link
Contributor Author

njucjc commented Aug 21, 2024 •
edited
Loading

@njucjc I watched the demo recording of the community meeting on July 24, 2024. Can you verify that the personal version of ACR is functioning properly at the same time?

@kofj Yes, the personal version of ACR can be working well at the same time.
image

image

@njucjc njucjc requested a review from kofj August 21, 2024 06:21
@njucjc njucjc mentioned this pull request Aug 26, 2024
Closed
@chlins
Copy link
Member

chlins commented Sep 2, 2024

@kofj Hi, do you have any other comments for this PR?

wy65701436
wy65701436 approved these changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@wy65701436 wy65701436 left a comment

Choose a reason for hiding this comment

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

lgtm

@chlins chlins merged commit 69bea4d into goharbor:main Oct 11, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chlins chlins chlins approved these changes

@wy65701436 wy65701436 wy65701436 approved these changes

@kofj kofj Awaiting requested review from kofj

Assignees

@stonezdj stonezdj

@wy65701436 wy65701436

@OrlinVasilev OrlinVasilev

@chlins chlins

@MinerYang MinerYang

Labels
release-note/new-feature New Harbor Feature target/2.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't support to sync image to ACR EE repositry
8 participants
@njucjc @wy65701436 @OrlinVasilev @kofj @chlins @stonezdj @AllForNothing @MinerYang