Skip to content

Conversation

SoumyaRaikwar
Copy link

@SoumyaRaikwar SoumyaRaikwar commented Sep 21, 2025

This PR addresses issue #22333.

Changes:

  • Drop REGISTRYURL variable and stop propagating it to sub-make.
  • Simplify make/photon/Makefile to always build the registry from source via the existing builder script.
  • Remove the Google Storage URL that was previously used to download a prebuilt registry binary.

Rationale:

  • The REGISTRYURL parameter and the related GCS download path are no longer used in the build pipeline.
  • Building the registry from source is the supported path and avoids relying on external binary artifacts.

Notes:

  • No Go code changes. CI should verify Makefile behavior remains intact.

@MinerYang
Copy link
Contributor

MinerYang commented Sep 22, 2025

Hi @SoumyaRaikwar ,

Thanks for contributing to harbor community.
However this Param is not unused. It will be consumed on demand.

Best,
Miner

@reasonerjt
Copy link
Contributor

@SoumyaRaikwar
Thank you for the PR. However, when checking the PR we find there's a misunderstanding in #22333, that this parm may still be needed in some custom cases when some downstream tries to download binaries from a 3rd server in the build process.

@wy65701436 please follow up and clarify.

SoumyaRaikwar pushed a commit to SoumyaRaikwar/harbor that referenced this pull request Sep 22, 2025
…binary (address review)

- Keep REGISTRYURL variable (empty by default) and pass-through to sub-make
- Restore conditional registry download path and _get_binary helper in photon Makefile
- Aligns with reviewer guidance on goharbor#22372

Signed-off-by: Your Name <[email protected]>
@SoumyaRaikwar
Copy link
Author

SoumyaRaikwar commented Sep 22, 2025

Thanks for the review. I’ve addressed the feedback:

  • Restored REGISTRYURL in the top-level Makefile (left empty by default).
  • Passed REGISTRYURL through to the photon sub-make build.
  • Restored the conditional download path in make/photon/Makefile for non-BUILDREG builds.
  • Restored _get_binary helper.
  • Kept removal of the hardcoded GCS URL.

@stonezdj Please take another look. Thanks!"

Copy link

codecov bot commented Sep 22, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.89%. Comparing base (c8c11b4) to head (bd2f197).
⚠️ Report is 569 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main   #22372       +/-   ##
===========================================
+ Coverage   45.36%   65.89%   +20.52%     
===========================================
  Files         244     1072      +828     
  Lines       13333   115932   +102599     
  Branches     2719     2927      +208     
===========================================
+ Hits         6049    76394    +70345     
- Misses       6983    35306    +28323     
- Partials      301     4232     +3931     
Flag Coverage Δ
unittests 65.89% <ø> (+20.52%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 986 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

thanks for your contribution. LGTM

@wy65701436 wy65701436 added the release-note/update Update or Fix label Sep 22, 2025
@SoumyaRaikwar
Copy link
Author

Hi! @stonezdj , could you please review my pr.

Copy link
Contributor

@bupd bupd left a comment

Choose a reason for hiding this comment

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

lgtm

@SoumyaRaikwar
Copy link
Author

SoumyaRaikwar commented Sep 23, 2025

Hi! @stonezdj @Vad1mo ,could you please review my pr ?

@wy65701436 wy65701436 enabled auto-merge (squash) September 25, 2025 07:43
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

@SoumyaRaikwar
Copy link
Author

@stonezdj could you please review my pr

Your Name added 2 commits September 29, 2025 16:20
…uild

Closes goharbor#22333

- Drop REGISTRYURL variable and its propagation
- Simplify make/photon/Makefile to always build registry from source via builder
- Remove Google Storage URL reference used for downloading prebuilt registry binary

Signed-off-by: Your Name <[email protected]>
…binary (address review)

- Keep REGISTRYURL variable (empty by default) and pass-through to sub-make
- Restore conditional registry download path and _get_binary helper in photon Makefile
- Aligns with reviewer guidance on goharbor#22372

Signed-off-by: Your Name <[email protected]>
Signed-off-by: Soumya Raikwar <[email protected]>
Signed-off-by: Soumya Raikwar <[email protected]>
Signed-off-by: Soumya Raikwar <[email protected]>
@stonezdj stonezdj force-pushed the 22333-remove-unused-registryurl branch from 76de3b0 to 0952395 Compare September 29, 2025 08:20
@SoumyaRaikwar
Copy link
Author

Hi @stonezdj @MinerYang @Vad1mo,

Could you please approve the workflow runs for this PR?

@SoumyaRaikwar
Copy link
Author

@bupd could you please approve the workflows run on my pr ?

@SoumyaRaikwar
Copy link
Author

@wy65701436 @stonezdj The PR has been approved and auto-merge is enabled, but it seems to be waiting on workflow approval. Could you please approve the CI runs when you get a chance? Thank you!

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.

7 participants