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: adds an upload artifacts command to konvoy image builder #214

Merged
merged 2 commits into from
Jan 28, 2022

Conversation

faiq
Copy link
Collaborator

@faiq faiq commented Jan 24, 2022

What problem does this PR solve?:
Creates an upload artifacts command to upload artifacts to hosts specified in an inventory file.

Which issue(s) does this PR fix?:

https://jira.d2iq.com/browse/D2IQ-83933

Special notes for your reviewer:

I ran the command like so:

$ bin/konvoy-image-wrapper upload artifacts --inventory-file=ansible-inventory.yaml --container-images-dir=./artifacts/image-bundles/ --os-packages-bundle=./artifacts/os-packages_1.21.6_x86_64_rpms.tar.gz --pip-packages-bundle=./artifacts/pip-packages.tar.gz 

I used previous existing terraform scripts to bring up centos7 hosts.

Does this PR introduce a user-facing change?:

feat: adds a upload artifacts command for hosts that will be used in offline mode.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 24, 2022

File Coverage
All files 3%
pkg/ansible/runner.go 0%
pkg/app/artifacts.go 0%
pkg/app/build.go 7%
pkg/app/errors.go 0%
pkg/app/provision.go 0%
pkg/app/root.go 0%
pkg/app/validate.go 0%
pkg/appansible/io.go 0%
pkg/appansible/playbook.go 0%
pkg/logging/logger.go 0%
pkg/packer/manifest.go 0%
pkg/packer/packer.go 0%
pkg/stringutil/rand.go 0%
pkg/version/info.go 0%

Minimum allowed coverage is 75%

Generated by 🐒 cobertura-action against 27cd289

@faiq faiq force-pushed the faiq/upload-artifacts-command branch 5 times, most recently from f6413b1 to e645bcf Compare January 24, 2022 20:48
@supershal
Copy link
Collaborator

Please add related target in Makefile. I think the e2e.*.offline targets will change to support this.

@supershal
Copy link
Collaborator

supershal commented Jan 24, 2022

for next iteration: Should we think about combining all artifacts into a single versioned tar if we need to provide all three type of artifacts (rpms, pip packages, images) all the time? This could prevent version mismatching between rpms and images artifacts. also it would be easier for users to manage/download one single tar file. Then this command could become simple where it could upload/sync sort of anything (for ex. Helm charts) to each host/bastion in the inventory.

Edit: Need to take care of OS specific artifacts Vs OS independent artifacts


func init() {
fs := artifactsCmd.Flags()
fs.StringVar(&artifactsFlags.Inventory, "inventory-file", "inventory.yaml", "an ansible inventory defining your infrastructure")
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should add shorthands notations to all commands. wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These should probably be addressed in a refactor for all of the subcommands.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sounds good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if we are following kubectl conventions, it would be good to follow those when we refactoring flags.
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-cli/kubectl-conventions.md#flag-conventions

@faiq faiq self-assigned this Jan 25, 2022
@@ -29,7 +29,6 @@
- include_vars: vars/flatcar/flatcar.yaml
when: ansible_os_family == "Flatcar"
roles:
- role: offline
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should actually keep this here. In the case where the user is trying to build an AMI with the offline bundles present

@faiq faiq force-pushed the faiq/upload-artifacts-command branch from e645bcf to d648476 Compare January 25, 2022 21:54
@faiq faiq removed the do not merge label Jan 25, 2022
Copy link
Collaborator

@supershal supershal left a comment

Choose a reason for hiding this comment

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

this is going to be a very useful feature.

docs/cli/konvoy-image_upload_artifacts.md Outdated Show resolved Hide resolved
Co-authored-by: John Baker <jbaker@d2iq.com>
@faiq faiq force-pushed the faiq/upload-artifacts-command branch from 942ff76 to 27cd289 Compare January 27, 2022 21:06
@faiq faiq merged commit 9ed1806 into main Jan 28, 2022
@faiq faiq deleted the faiq/upload-artifacts-command branch January 28, 2022 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants