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

feat(dl): support download file with file_regex param #169

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

wuhuizuo
Copy link
Contributor

example:

curl -v "http://localhost:8000/oci-file/hub.pingcap.net/pingcap/tidb/package?tag=v8.3.0_linux_amd64&
file_regex=tidb-lightning-.%2A%5B.%5Dsha256"

to download the file with regex pattern tidb-lightning-.*[.]sha256.

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

@ti-chi-bot ti-chi-bot bot requested a review from purelind September 18, 2024 13:53
@ti-chi-bot ti-chi-bot bot added the size/XL label Sep 18, 2024
Copy link

ti-chi-bot bot commented Sep 18, 2024

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

The key changes in this pull request are as follows:

  1. The download-file method in the oci service is updated to support downloading files with a file_regex param. This param is used to match file names based on a regular expression.

  2. The file param in the download-file method is no longer required. It is now optional and can be used alongside file_regex. If file is provided, the service will attempt to download the file with the given name. If file_regex is provided, the service will attempt to download the first file that matches the regular expression.

  3. The example usage in the CLI tool is updated to reflect these changes.

Potential problems:

  1. There's no validation for the case when both file and file_regex params are provided. It's unclear from the changes what the expected behavior should be in such cases. It could potentially lead to confusion or bugs.

  2. In BuildDownloadFilePayload function, the error from ValidateFormat for file_regex is being merged with the previous err, but this previous err is always nil in the given context. So, the MergeErrors function call is unnecessary.

  3. The file_regex param is not thoroughly validated. While it's checked if it's a valid regular expression, there's no check if it's an empty string. An empty regular expression will match everything, which could lead to unexpected behavior.

Fixing suggestions:

  1. Define and document the behavior when both file and file_regex params are provided. For example, you could decide that file takes precedence and ignore file_regex in such cases, or return an error to the user.

  2. Remove the unnecessary MergeErrors function call in BuildDownloadFilePayload.

  3. Add validation for file_regex to ensure it's not an empty string. If an empty string is not a valid input, the API should return an error when it's provided.

example:

curl -v "http://localhost:8000/oci-file/hub.pingcap.net/pingcap/tidb/package?tag=v8.3.0_linux_amd64&
file_regex=tidb-lightning-.%2A%5B.%5Dsha256"

to download the file with regex pattern `tidb-lightning-.*[.]sha256`.

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@wuhuizuo wuhuizuo force-pushed the feature/dl-support-download-with-regex-match branch from 52acf1b to 5926074 Compare September 18, 2024 13:53
Copy link

ti-chi-bot bot commented Sep 18, 2024

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

The PR is about the feature to support downloading a file with a file_regex parameter in the specified project. The file_regex parameter allows users to download a file matching a certain regular expression pattern.

Here are some key changes:

  1. The version of the Golang Docker image has been updated from 1.22.4 to 1.23.1.
  2. Changes have been made in the dl/design/design.go file to incorporate the file_regex parameter into the method's payload and the HTTP request.
  3. The file parameter is no longer required, hence a check is added to handle the case when this parameter is not provided.
  4. The file_regex parameter is validated against a regular expression format for correctness.
  5. The request parameters in the dl/gen/http/cli/server/cli.go file have been updated to include the file_regex parameter.
  6. The code for building the payload and encoding/decoding the request has been updated to handle the file_regex parameter in the dl/gen/http/oci/client/cli.go and related files.
  7. The OpenAPI specification has been updated to reflect the new file_regex parameter.

Potential Problems:

  1. One potential problem could be the regular expression validation. If the provided file_regex doesn't match the required format, it could lead to errors.
  2. If the file and file_regex parameters are both not provided, it might lead to an error. This case is handled, but needs to be tested for robustness.
  3. The method now supports two ways of file selection (file and file_regex). If both parameters are provided, the current implementation prefers file. If the intended behavior is to give preference to file_regex, the logic would need to be updated.

Suggestions for fixes:

  1. More extensive testing should be done, especially to handle edge cases related to the file_regex parameter.
  2. Consider adding more detailed comments for the changes in functions and methods, explaining the purpose of the changes.
  3. Make sure that the new feature is properly documented so that users know that they can now use a regex to download a file.
  4. It would be beneficial to add some error handling for the case when both file and file_regex parameters are provided. You could return an error to the user, or clearly document which parameter takes precedence.

@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Sep 18, 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 Sep 18, 2024
@ti-chi-bot ti-chi-bot bot merged commit 09f428e into main Sep 18, 2024
3 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feature/dl-support-download-with-regex-match branch September 18, 2024 14:01
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