-
Notifications
You must be signed in to change notification settings - Fork 226
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
Update Diffing Display #1419
Update Diffing Display #1419
Conversation
- update upstream directory names - add logging of staged directories
- reduces the length of diffed directories - uses short commit sha or up to 7 branch characters
is there a test we need to develop or update to make sure we don't regress this functionality in the future? |
We do not currently have tests over the specific parts of code I've touched here. However there are a few tests covering the complete diff functionality. I'm working on developing tests in parallel but wanted to create this first to get feedback on the experience it creates. |
- refactor shortSha - use one staging directory instead of three
internal/util/diff/diff.go
Outdated
kptFile.Upstream.Git.Repo, | ||
kptFile.Upstream.Git.Directory, | ||
kptFile.Upstream.Git.Commit, | ||
upstreamPkg) |
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.
same question as above. Do people expect to see the directory where things are staged after the "at..."
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.
The pkg
variables store the path to the directory containing the staged package. This may be refactored in the future. The path is returned from the stageDirectory
and GetPkg
functions.
} | ||
|
||
// defaultPkgGetter uses get.Command abstraction to implement PkgGetter. | ||
type defaultPkgGetter struct{} | ||
|
||
func (pg defaultPkgGetter) GetPkg(repo, path, ref string) (string, error) { | ||
dir, err := ioutil.TempDir("", "kpt-") | ||
// GetPkg checks out a repository into a temporary directory for diffing |
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.
<3 the extra comments. Thanks for relaying the context!
- add accounting for some extra edge cases
After latest changes directories will be staged similar to:
The
|
* Update diff info - update upstream directory names * Distinguish local and remote in dir names * Introduce a descriptive reference name in dir * Rename diffed directories - reduces the length of diffed directories - uses short commit sha or up to 7 branch characters * Update local diff path. * Create shared staging directory - refactor shortSha - use one staging directory instead of three * Add error handling for staging directory * Place staging dir naming under test - add accounting for some extra edge cases
* Update Diffing Display (#1419) * Update diff info - update upstream directory names * Distinguish local and remote in dir names * Introduce a descriptive reference name in dir * Rename diffed directories - reduces the length of diffed directories - uses short commit sha or up to 7 branch characters * Update local diff path. * Create shared staging directory - refactor shortSha - use one staging directory instead of three * Add error handling for staging directory * Place staging dir naming under test - add accounting for some extra edge cases * Update tests * Remove extra comment * Add help message when diff fails (#1466) * Add help message when diff fails * Remove unused code * Address Pr comments * Refactor tests * Update test to validate arguments * Fix linting error
* Update Diffing Display (kptdev#1419) * Update diff info - update upstream directory names * Distinguish local and remote in dir names * Introduce a descriptive reference name in dir * Rename diffed directories - reduces the length of diffed directories - uses short commit sha or up to 7 branch characters * Update local diff path. * Create shared staging directory - refactor shortSha - use one staging directory instead of three * Add error handling for staging directory * Place staging dir naming under test - add accounting for some extra edge cases * Update tests * Remove extra comment * Add help message when diff fails (kptdev#1466) * Add help message when diff fails * Remove unused code * Address Pr comments * Refactor tests * Update test to validate arguments * Fix linting error
Attempts to improve the UX of kpt package diffs like experienced in #1377
update upstream directory names
kpt-12348765
might now appear askpt-upstream-examples-cockroachdb-ProjectVersion-12348765
where:upstream
references that the package was fetched from the source package. The local copy will appear aslocal
instead.examples
references the Git project name. In this case taken from: https://github.com/kubernetes/examplescockroachdb
references the directory containing the upstream package. In this case taken from: staging/cockroachdbProjectVersion
is a string constant that represents a version stored in the projects Kptfile. During remote or 3way diffs this may also appear as the provided reference (main
,master
etc).add output of staged directories to stdout, this information looks like this:
PackageVersion
represents a package which matches the package version stored in the projects Kptfilemaster
will represent the remote reference used during a 3way diff.There are some known issues with this approach: