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

Fix cacheTo Function/container prop #3722

Merged
merged 2 commits into from Apr 27, 2024
Merged

Conversation

adrianisk
Copy link
Contributor

Summary

Small fix for my earlier PR #3651 that added buildSsh, cacheTo, cacheFrom properties to Functions/containers

My .join(",") was in the wrong place, and was joining the characters of each string in the array instead of the array of strings which produced Docker build arguments like

--cache-to=type=local,d,e,s,t,=,.,,,m,o,d,e,=,m,a,x

Tests

I updated the test I added to include additional params in the cacheTo object to verify things now work as expected.

Before

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

FAIL  test/constructs/Function.test.ts > runtime: container: props
Error: Failed to build function "test/constructs/container-function"
Error: Command failed: docker build -t sst-build:c8ed9dbe9a800c218512b749c670e088ecf3a53842 --cache-from=type=inline --cache-from=type=local,src=.,mode=max --cache-to=type=local,d,e,s,t,=,.,,,m,o,d,e,=,m,a,x --platform linux/amd64 .
ERROR: invalid value d

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯

Test Files  1 failed (1)
    Tests  1 failed | 109 passed (110)
  Start at  18:14:18
  Duration  11.54s (transform 366ms, setup 0ms, collect 1.89s, tests 9.41s, environment 0ms, prepare 60ms)

After

Test Files  1 passed (1)
   Tests  110 passed (110)
  Start at  18:15:21
  Duration  11.88s (transform 352ms, setup 0ms, collect 1.92s, tests 9.73s, environment 0ms, prepare 59ms)   

Copy link

changeset-bot bot commented Mar 23, 2024

🦋 Changeset detected

Latest commit: ab904a0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
sst Patch
@sst/console Patch
create-sst Patch
astro-sst Patch
svelte-kit-sst Patch
solid-start-sst Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jayair
Copy link
Contributor

jayair commented Mar 26, 2024

Taking a look

@arturenault
Copy link

Hi, what's preventing this PR from being merged? We're hitting this issue.

@jayair
Copy link
Contributor

jayair commented Apr 26, 2024

Hmm we might've missed this. I'll ping the team.

@fwang fwang merged commit 6f8fb48 into sst:master Apr 27, 2024
1 check was pending
@fwang
Copy link
Contributor

fwang commented Apr 27, 2024

Thanks @adrianisk!

Will be in the next release.

@arturenault
Copy link

Thanks folks! When can we expect the next release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants