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

imgpkg Refactoring #24

Merged
merged 8 commits into from
Dec 8, 2020
Merged

imgpkg Refactoring #24

merged 8 commits into from
Dec 8, 2020

Conversation

danielhelfand
Copy link
Contributor

This is a refactor of imgpkg to address the following issues:

  • Removal of any unused code brought over from kbld
  • Cleaning up the switch statement in GetUnprocessedURLs in cmd/copy.go to remove problems of DRY
  • Moving ImagesLock and BundleLock into a separate package outside of cmd (Extract lock files to their own packages #17)
  • Creation of the concept of a Bundle via a Bundle struct
  • Switching the YAML dependency (Move to k8s yaml package #16)
  • General cleanup around all imgpkg code (correcting typos, updating language in error messages, adding tests)

Co-Authored-By: gcheadle-vmware <67297250+gcheadle-vmware@users.noreply.github.com>
@joaopapereira
Copy link
Member

Closes #17, #16

Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

The PR looks good. I added some minor changes, especially around the imports.
The Switch refractory looks good, nevertheless, it still feels a little big (I think we can address that in the future).
Personally, I would prefer to use fewer constants in our tests to ensure that the change I pointed out would be more easily detected.

pkg/imgpkg/cmd/push_test.go Outdated Show resolved Hide resolved
pkg/imgpkg/lockfiles/lock_files.go Outdated Show resolved Hide resolved
pkg/imgpkg/lockfiles/bundle_test.go Outdated Show resolved Hide resolved
pkg/imgpkg/lockfiles/lock_files.go Show resolved Hide resolved
test/e2e/copy_test.go Outdated Show resolved Hide resolved
pkg/imgpkg/cmd/copy.go Outdated Show resolved Hide resolved
danielhelfand and others added 5 commits December 4, 2020 17:47
Co-authored-by: João Pereira <joaod@vmware.com>
Co-authored-by: João Pereira <joaod@vmware.com>
Co-authored-by: João Pereira <joaod@vmware.com>
Co-authored-by: João Pereira <joaod@vmware.com>
@danielhelfand
Copy link
Contributor Author

Just wanted to leave a comment here that perhaps it would make sense to discuss introducing this refactor after the release is cut.

Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

looks good to me

@pivotaljohn
Copy link
Contributor

(git-history, if-feasible) Could we squash any commits that fall under the same intent?

@danielhelfand
Copy link
Contributor Author

(git-history, if-feasible) Could we squash any commits that fall under the same intent?

Not sure if feasible, but can keep in mind for future prs to be more mindful of commit history.

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.

4 participants