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

perf(packages/scripts): improve efficiently of fetching file from artifacts #217

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

wuhuizuo
Copy link
Contributor

not pull all files, but just the needed file.

Signed-off-by: wuhuizuo wuhuizuo@126.com

@ti-chi-bot ti-chi-bot bot requested review from jayl1e and purelind January 22, 2024 07:26
Copy link

ti-chi-bot bot commented Jan 22, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:

This pull request aims to improve the efficiency of fetching files from artifacts by not pulling all files but just the needed file. To achieve this goal, the PR adds a new function called fetch_file_from_oci_artifact(). The function retrieves the file blob digest and downloads the file from the OCI artifact.

Potential problems:

  1. The fetch_file_from_oci_artifact() function is not tested, which can lead to bugs in the future.
  2. The build_and_push_images() function in build-package-images.sh.tmpl still has the old code that pulls all files from OCI repositories. It should be removed to avoid confusion.

Suggestions:

  1. Add unit tests to the fetch_file_from_oci_artifact() function to ensure its reliability.
  2. Remove the old code that pulls all files from OCI repositories in build_and_push_images().

Overall, the changes in this pull request look good, but it would be better to address the above-mentioned issues before merging it.

@ti-chi-bot ti-chi-bot bot added the size/M label Jan 22, 2024
…ifacts

not pull all files, but just the needed file.

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Copy link

ti-chi-bot bot commented Jan 22, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:

  • The pull request aims to improve the efficiency of fetching files from artifacts.
  • The changes include a new function fetch_file_from_oci_artifact, which is used to get the specific file blob digest and download the file from the OCI repository.
  • The oras pull command is replaced with fetch_file_from_oci_artifact in the archive() and compose_artifact() functions.

Potential problems:

  • The pull request does not include any tests to ensure that the new function works as expected.
  • The build_and_push_images() function in build-package-images.sh.tmpl has a new assertion that len $ociFiles should be 0, which seems unrelated to the changes made in this pull request. This assertion should be moved to a separate pull request.

Fixing suggestions:

  • Add tests to validate the new fetch_file_from_oci_artifact function.
  • Move the assertion in build_and_push_images() to a separate pull request.
  • Consider refactoring the function names to make them more descriptive and consistent with the project's naming conventions.

@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Jan 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the approved label Jan 22, 2024
@ti-chi-bot ti-chi-bot bot merged commit d02b34a into main Jan 22, 2024
1 of 2 checks passed
@ti-chi-bot ti-chi-bot bot deleted the fix/fix-oci-performance branch January 22, 2024 07:40
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.

1 participant