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

Fixes and tests for ingredientcall end-to-end #3575

Merged
merged 43 commits into from
Nov 13, 2024
Merged

Conversation

Naatan
Copy link
Member

@Naatan Naatan commented Oct 31, 2024

No description provided.

@Naatan Naatan changed the base branch from master to version/0-47-0-RC1 October 31, 2024 20:41
for _, glob := range globs {
files, err := filepath.Glob(glob)
files, err := doublestar.Glob(fs, glob)
Copy link
Member Author

Choose a reason for hiding this comment

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

filepath.Glob() does not support double star globs (ie. */**) nor specifying the working directory. This library addresses both limitations.

Comment on lines +106 to +111
// Ensure the overall hash is consistently calculated
sort.Slice(hashedFiles, func(i, j int) bool { return hashedFiles[i].Path < hashedFiles[j].Path })
h := xxhash.New()
for _, f := range hashedFiles {
fmt.Fprintf(h, "%016x", f.Hash)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it to calculate the overall hash based on a sorted slice of all the individual hashes for all files. This way we get the same hash even if the files are sorted differently, or more importantly (what I ran into); if the patterns are different but produce the same result.

Target string
}

func CreateTgz(filepath string, fileMaps []FileMap) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

Archiver supports creating archives, but its API is pointlessly convoluted. This package is just a wrapper that doesn't require us to write so much boilerplate just to create a simple archive.

@Naatan Naatan requested a review from MDrakos October 31, 2024 21:48
@Naatan Naatan requested a review from MDrakos November 1, 2024 20:33
MDrakos
MDrakos previously approved these changes Nov 1, 2024
@@ -172,7 +175,8 @@ func unmarshalValue(path []string, valueInterface interface{}) (*value, error) {
result.List = &values

case string:
if sliceutils.Contains(path, ctxIn) || strings.HasPrefix(v, "$") {
parentNode, hasParentNode := sliceutils.GetString(path, -2)
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

@Naatan Naatan requested a review from MDrakos November 7, 2024 22:19
MDrakos
MDrakos previously approved these changes Nov 8, 2024
Copy link
Member

@MDrakos MDrakos 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, just one comment to consider.

for _, fileMap := range fileMaps {
source := fileMap.Source
if !filepath.IsAbs(source) {
source = filepath.Join(workDir, source)
Copy link
Member

Choose a reason for hiding this comment

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

From reading the code in this file it's not clear to me why we're appending the source path to the workDir rather than just using the absolute path here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't follow, this is what is making the path absolute. Are you seeing another absolute path value that I can use?

Copy link
Member

Choose a reason for hiding this comment

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

Ahh okay, maybe a comment on the FileMap struct would help here? I was expecting to see a call to filepath.Abs if the path isn't absolute but if that's how the file map is constructed it makes sense as well.

Copy link
Member Author

@Naatan Naatan Nov 8, 2024

Choose a reason for hiding this comment

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

filepath.Abs() I believe works with the working directory, which is a global so something we want to avoid. I'll add a comment to clarify. Fwiw the reason the paths are relative is to facilitate archiving, otherwise the archives also have absolute paths in them, ie. rather than having setup.py at the root of the archive it'd be at c:\Users\Runner~1\ etc. (oddly specific example that totally does not indicate I struggled with this..).

MDrakos
MDrakos previously approved these changes Nov 8, 2024
@Naatan Naatan merged commit fbf7ca2 into version/0-47-0-RC1 Nov 13, 2024
7 of 8 checks passed
@Naatan Naatan deleted the DX-3105 branch November 13, 2024 17:13
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.

2 participants