-
Notifications
You must be signed in to change notification settings - Fork 817
feat: add web installer in artifact export func #2905
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
base: main
Are you sure you want to change the base?
feat: add web installer in artifact export func #2905
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zuoxuesong-worker The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary of ChangesHello @zuoxuesong-worker, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the artifact export functionality by automating the inclusion of the web installer in offline packages. Previously, users had to follow a multi-step manual process to generate these packages. The changes streamline this workflow by integrating the web installer's download, extraction, and configuration directly into the artifact export playbook, making the process more efficient and user-friendly. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds functionality to include a web installer in the artifact export process, which is a great feature. My review focuses on improving the implementation by adhering to Ansible best practices. I've suggested replacing shell commands with Ansible's built-in modules like file, copy, archive, get_url, and unarchive. These changes will make the playbooks more idempotent, reliable, and easier to maintain. I also found a minor issue with file permissions that should be corrected.
| command: >- | ||
| mkdir -p {{ .artifact_file_dir }}/kubekey/kubekey/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| command: | | ||
| cp kk {{ .artifact_file_dir }}/ | ||
| cd {{ .artifact_file_dir }} && tar -czvf {{ .artifact_file }} * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using command with cp and tar is not idempotent and is less readable than using Ansible's dedicated modules. I suggest refactoring this to use the copy and archive modules. This improves idempotency, error handling, and clarity. I've wrapped them in a block to group the related actions.
block:
- name: Artifact | Copy kk binary to artifact dir
copy:
src: kk
dest: "{{ .artifact_file_dir }}/"
remote_src: yes
- name: Artifact | Create artifact archive
archive:
path: "{{ .artifact_file_dir }}/"
dest: "{{ .artifact_file }}"
format: gz| command: | | ||
| curl -L -o {{ .artifact_file_dir }}/web-installer.tgz {{ .download.web_installer.url }} | ||
| tar -xzf "{{ .artifact_file_dir }}/web-installer.tgz" --no-same-owner -C {{ .artifact_file_dir }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using command with curl and tar, it's highly recommended to use the get_url and unarchive modules. These modules are idempotent, provide better error handling, and make the playbook more readable and maintainable. I've replaced the command with a block containing these modules so they share the same when condition.
block:
- name: Download web installer
get_url:
url: "{{ .download.web_installer.url }}"
dest: "{{ .artifact_file_dir }}/web-installer.tgz"
- name: Extract web installer
unarchive:
src: "{{ .artifact_file_dir }}/web-installer.tgz"
dest: "{{ .artifact_file_dir }}"
remote_src: yes
extra_opts: --no-same-owner| template: | ||
| src: config.json | ||
| dest: "{{ .artifact_file_dir }}/schema/config.json" | ||
| mode: 0755 No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
73a309c to
736c63b
Compare
| pre_tasks: | ||
| - name: Artifact | Set artifact file dir | ||
| when: | ||
| - .artifact_file | empty | not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it may be download.artifact_file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kubekey/cmd/kk/app/options/option.go
Line 238 in 238eb2b
| if err := unstructured.SetNestedField(o.Config.Value(), o.Artifact, "artifact_file"); err != nil { |
no , it IS artifact_file
| post_tasks: | ||
| - name: Artifact | Export artifact | ||
| when: | ||
| - .pack_artifact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what‘s this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
artifact file will store in artifact_file_path before pack , add an arg named pack_artifact gave user a choice wheather pack artifact_file or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when user want to make a package with multi version k8s , then user can do
./kk artifact export -c first.yaml --workdir prepare -a $(pwd)/artifact.tgz
./kk artifact export -c second.yaml --workdir prepare -a $(pwd)/artifact.tgz
./kk artifact export -c last.yaml --workdir prepare -a $(pwd)/artifact.tgz --set pack_artifact=true,download.web_installer.download_web_installer=true
kk will download web-installer and make tgz only in the last cmd executed
736c63b to
8a978cd
Compare
Signed-off-by: xuesongzuo@yunify.com <xuesongzuo@yunify.com> feat: add web installer in artifact export func Signed-off-by: xuesongzuo@yunify.com <xuesongzuo@yunify.com> feat: add web installer in artifact export func Signed-off-by: xuesongzuo@yunify.com <xuesongzuo@yunify.com> feat: add web installer in artifact export func Signed-off-by: xuesongzuo@yunify.com <xuesongzuo@yunify.com> feat: add web installer in artifact export func Signed-off-by: xuesongzuo@yunify.com <xuesongzuo@yunify.com> feat: add web installer in artifact export func Signed-off-by: xuesongzuo@yunify.com <xuesongzuo@yunify.com> feat: add web installer in artifact export func Signed-off-by: xuesongzuo@yunify.com <xuesongzuo@yunify.com> feat: add web installer in artifact export func Signed-off-by: xuesongzuo@yunify.com <xuesongzuo@yunify.com> feat: add web installer in artifact export func Signed-off-by: xuesongzuo@yunify.com <xuesongzuo@yunify.com> feat: add web installer in artifact export func Signed-off-by: xuesongzuo@yunify.com <xuesongzuo@yunify.com> feat: add web installer in artifact export func Signed-off-by: xuesongzuo@yunify.com <xuesongzuo@yunify.com> feat: add web installer in artifact export func Signed-off-by: xuesongzuo@yunify.com <xuesongzuo@yunify.com> feat: add web installer in artifact export func Signed-off-by: xuesongzuo@yunify.com <xuesongzuo@yunify.com>
8a978cd to
a2d45d5
Compare
|


feat: add web installer in artifact export func
What type of PR is this?
/kind feature
What this PR does / why we need it:
Now when we create an offline package ,we should use get kk first and generate package.sh file
then exec this sh file to get an package and web-installer
it makes get kk complicated
update kk , add web-installer downloader and pack func
Which issue(s) this PR fixes:
Fixes #
Special notes for reviewers:
Does this PR introduced a user-facing change?
Additional documentation, usage docs, etc.: