feat(cli): support short-syntax referencing for archives#618
feat(cli): support short-syntax referencing for archives#618jakobmoellerdev wants to merge 3 commits into
Conversation
- Added logic to recognize and exclude archive-like file paths (e.g., `.tar`, `.tgz`) from being incorrectly identified as domains. - Introduced a utility function `looksLikeArchive` to validate archive paths against a set of whitelisted extensions (`.tar`, `.tgz`, `.tar.gz`). - Extended test coverage to include cases involving archive paths. This enhancement ensures that archive paths are appropriately handled in short-syntax references, improving the accuracy of path resolution. Signed-off-by: Jakob Möller <jakob.moeller@sap.com>
b9a85b3 to
fd152b8
Compare
…uction - Introduced tests to validate support for `.tar` and `.tar.gz` archive formats in component version construction. - Ensured proper handling of working directory and YAML-based construction for archive inputs. - Improved robustness of the CLI's archive-related functionality with additional test cases. Signed-off-by: Jakob Möller <jakob.moeller@sap.com>
Signed-off-by: Jakob Möller <jakob.moeller@sap.com>
| // be interpreted as path, it needs to be passed explicitly | ||
| func looksLikeArchive(s string) bool { | ||
| for _, ext := range allowListArchiveFilePathExtensions { | ||
| if strings.HasSuffix(s, ext) { |
There was a problem hiding this comment.
Wouldn't it be a bit more reliable if we did an archive check via reading tar or something? You know something simple like this:
// isTar checks if a given content is a tar archive or not.
func isTar(content []byte) bool {
tr := tar.NewReader(bytes.NewBuffer(content))
_, err := tr.Next()
return err == nil
}There was a problem hiding this comment.
hm, I guess that would require to have the content here.
There was a problem hiding this comment.
I guess I was hoping to not have to open a file descriptor on here. I could do that of course in theory, but that would make the operation significantly more expensive. WDYT?
There was a problem hiding this comment.
I'd say let's roll with the looksLikeArchive logic - it is very simple and maintainable, and therefore, a very practical solution for the current problem.
Besides, I think relying on file extensions is a quite common thing to do.
| text: foobar | ||
| `) | ||
|
|
||
| // Create a replacement test file to be added to the component version |
There was a problem hiding this comment.
I don't understand that comment
| "--constructor", constructorYAMLFilePath, | ||
| "--repository", filepath.Join("transport-archive", tc), | ||
| "--working-directory", tmp, | ||
| ), test.WithOutput(logs)) |
There was a problem hiding this comment.
We aren't using the logs, are we?
| t.Run("archives", func(t *testing.T) { | ||
| tcs := []string{ | ||
| ".tar", | ||
| ".tar.gz", | ||
| } |
There was a problem hiding this comment.
I feel like this test should not be a table test. The table test should happen based on the allow list in the compref packages unit tests.
There was a problem hiding this comment.
so it should be a table test but use the allow list as entries? :D
There was a problem hiding this comment.
Sorry - might have been a little vague. I think the table test makes sense. But it should not be a table test in the e2e cli tests but rather in the reference parsing tests. Don't you think so?
|
Closing in favor of a properly tested implementation as a learning opportunity |
What this PR does / why we need it
.tar,.tgz) from being incorrectly identified as domains.looksLikeArchiveto validate archive paths against a set of whitelisted extensions (.tar,.tgz,.tar.gz).Which issue(s) this PR fixes
This enhancement ensures that archive paths are appropriately handled in short-syntax references, improving the accuracy of path resolution.