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

chore(dockerfiles/cd/utils/release): pretty output for offline package steps #352

Merged
merged 2 commits into from
Jul 9, 2024

Conversation

wuhuizuo
Copy link
Contributor

@wuhuizuo wuhuizuo commented Jul 9, 2024

User description

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


PR Type

enhancement, bug fix


Description

  • Removed profile attribute from enterprise server package definitions in packages/offline-packages.yaml.tmpl.
  • Added -oy flag to yq command for pretty output in fetch_file_from_oci_artifact function in packages/scripts/build-package-artifacts.sh.tmpl.
  • Added -oy flag to yq command for pretty output in fetch_file_from_oci_artifact and check_file_in_oci_artifact functions in packages/scripts/compose-offline-packages-artifacts.sh.tmpl.

Changes walkthrough 📝

Relevant files
Enhancement
offline-packages.yaml.tmpl
Remove profile attribute from enterprise server packages 

packages/offline-packages.yaml.tmpl

  • Removed profile attribute from enterprise server package definitions.
  • +0/-2     
    build-package-artifacts.sh.tmpl
    Add pretty output flag to yq command in build script         

    packages/scripts/build-package-artifacts.sh.tmpl

  • Added -oy flag to yq command for pretty output in
    fetch_file_from_oci_artifact function.
  • +1/-1     
    compose-offline-packages-artifacts.sh.tmpl
    Add pretty output flag to yq command in compose script     

    packages/scripts/compose-offline-packages-artifacts.sh.tmpl

  • Added -oy flag to yq command for pretty output in
    fetch_file_from_oci_artifact and check_file_in_oci_artifact functions.

  • +2/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @ti-chi-bot ti-chi-bot bot requested a review from purelind July 9, 2024 11:33
    Copy link

    ti-chi-bot bot commented Jul 9, 2024

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

    Based on the title and diff, it seems that the pull request is related to improving the output of offline package steps in Dockerfiles. More specifically, the changes made to the pull request replace --prettyPrint with -oy in two shell scripts (build-package-artifacts.sh.tmpl and compose-offline-packages-artifacts.sh.tmpl) to provide pretty output for offline package steps.

    There are no potential problems with this pull request. The changes made are minor but helpful in terms of improving the readability of the output of offline package steps.

    Suggestion: As the changes made in the pull request are minor, I suggest merging the pull request once it's approved.

    @ti-chi-bot ti-chi-bot bot added the size/XS label Jul 9, 2024
    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot added enhancement New feature or request Bug fix labels Jul 9, 2024
    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug:
    The removal of the profile attribute from the packages/offline-packages.yaml.tmpl might affect existing configurations or dependencies that expect this attribute. Ensure that this change does not break any existing functionality or integrations.

    Code Consistency:
    The addition of the -oy flag to the yq command in multiple scripts for pretty output is consistent across the changes. However, ensure that the pretty output does not interfere with any downstream processing or logging mechanisms that might expect a different format.

    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Improve readability and consistency by using the -o option with yq

    Consider using the -o option with yq to specify the output format directly, which
    can improve readability and maintain consistency with other scripts.

    packages/scripts/build-package-artifacts.sh.tmpl [94]

    -oras manifest fetch $destination | yq -e --prettyPrint -oy ".layers | filter(.annotations[\"org.opencontainers.image.title\"] | test \"$to_match_file\") | .[0]" >blob.yaml
    +oras manifest fetch $destination | yq -e --prettyPrint -o yaml ".layers | filter(.annotations[\"org.opencontainers.image.title\"] | test \"$to_match_file\") | .[0]" >blob.yaml
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion improves readability and consistency by explicitly specifying the output format, which aligns with best practices and enhances maintainability.

    8
    Maintainability
    Use consistent yq output format options for better script maintainability

    Ensure that the yq command uses consistent output format options across different
    scripts for better maintainability.

    packages/scripts/compose-offline-packages-artifacts.sh.tmpl [79]

    -oras manifest fetch $destination | yq --prettyPrint -oy ".layers | filter(.annotations[\"org.opencontainers.image.title\"] | test \"$to_match_file\") | .[0]" >blob.yaml
    +oras manifest fetch $destination | yq --prettyPrint -o yaml ".layers | filter(.annotations[\"org.opencontainers.image.title\"] | test \"$to_match_file\") | .[0]" >blob.yaml
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Ensuring consistent use of yq output format options across scripts improves maintainability and readability, making the codebase easier to manage.

    8
    Enhancement
    Improve clarity and consistency in artifact naming

    Ensure that the artifact names are consistent and correctly formatted by using a
    more explicit templating syntax for clarity.

    packages/offline-packages.yaml.tmpl [367]

    -- name: "tidb-enterprise-server-{{ .Release.version }}-{{ .Release.os }}-{{ .Release.arch }}"
    +- name: "tidb-enterprise-server-{{ printf \"%s-%s-%s\" .Release.version .Release.os .Release.arch }}"
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a more explicit templating syntax for artifact names enhances clarity and consistency, which can help avoid errors and improve readability.

    7

    …e steps
    
    Signed-off-by: wuhuizuo <wuhuizuo@126.com>
    From oras v1.2.0, there's a break change for cli option of sub command `discover`.
    Ref: https://github.com/oras-project/oras/releases/tag/v1.2.0
    
    Signed-off-by: wuhuizuo <wuhuizuo@126.com>
    @wuhuizuo wuhuizuo force-pushed the fix/update-offline-pkg-config branch from 7e83838 to 78d8678 Compare July 9, 2024 12:30
    Copy link

    ti-chi-bot bot commented Jul 9, 2024

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

    This pull request includes changes related to improving the output of some commands in the Dockerfiles. Specifically, it removes the "profile" attribute from enterprise server packages and adds "-oy" flag to "yq" command for pretty output in the "fetch_file_from_oci_artifact" and "check_file_in_oci_artifact" functions.

    There are no potential problems identified in this pull request. However, there are some suggestions that could be made to further improve the changes. For instance, it would be beneficial to add more context to the PR description to help other reviewers understand the purpose of the changes. Additionally, it would be helpful to add some explanation of why the "profile" attribute was removed from enterprise server packages.

    Overall, this pull request looks good, and the changes should be merged once the suggestions are implemented.

    @ti-chi-bot ti-chi-bot bot added size/S and removed size/XS labels Jul 9, 2024
    @wuhuizuo
    Copy link
    Contributor Author

    wuhuizuo commented Jul 9, 2024

    depends on PingCAP-QE/ee-ops#1181

    @wuhuizuo
    Copy link
    Contributor Author

    wuhuizuo commented Jul 9, 2024

    /approve

    Copy link

    ti-chi-bot bot commented Jul 9, 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 Jul 9, 2024
    @ti-chi-bot ti-chi-bot bot merged commit 62eb57b into main Jul 9, 2024
    1 of 2 checks passed
    @ti-chi-bot ti-chi-bot bot deleted the fix/update-offline-pkg-config branch July 9, 2024 12:43
    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