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 compute pack to produce expected package.tar.gz file name #662

Merged
merged 2 commits into from
Oct 17, 2022

Conversation

Integralist
Copy link
Collaborator

@Integralist Integralist commented Oct 11, 2022

We updated the compute build and compute deploy commands to generate, and lookup, a package inside /pkg/package.tar.gz but we then neglected to update the compute pack command (which still had the old behaviour of dynamically generating the package filename, e.g. /pkg/<dynamic_name>.tar.gz).

@Integralist Integralist added the bug Something isn't working label Oct 11, 2022
@Integralist Integralist requested review from a team and Journie-Fastly and removed request for a team October 11, 2022 11:06
Copy link
Collaborator Author

@Integralist Integralist left a comment

Choose a reason for hiding this comment

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

Here are some comments to help guide the reviewer...

@@ -16,9 +16,7 @@ require (
github.com/getsentry/sentry-go v0.12.0
github.com/google/go-cmp v0.5.6
github.com/google/go-github/v38 v38.1.0
github.com/kennygrant/sanitize v1.2.4
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ℹ️ Now we no longer generate a package name from information in the user's fastly.toml manifest file, it means we don't need this dependency.

@@ -39,6 +39,8 @@ func (c *PackCommand) Exec(_ io.Reader, out io.Writer) (err error) {
progress := text.NewProgress(out, c.Globals.Verbose())

defer func(errLog fsterr.LogInterface) {
os.RemoveAll("pkg/package")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ℹ️ In the compute build command we create a 'temp' directory to store files that are placed into the final package.tar.gz, and then clean up the temp directory. So we are now doing the same thing here, and are just making sure we clean-up after ourselves.

Comment on lines +53 to +54
bin := "pkg/package/bin/main.wasm"
bindir := filepath.Dir(bin)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ℹ️ I'm just renaming variables for greater clarity, as the previous names were a bit too ambiguous.

@@ -91,7 +92,7 @@ func (c *PackCommand) Exec(_ io.Reader, out io.Writer) (err error) {

progress.Step("Copying manifest...")
src = manifest.Filename
dst = fmt.Sprintf("pkg/%s/%s", name, manifest.Filename)
dst = fmt.Sprintf("pkg/package/%s", manifest.Filename)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ℹ️ Here is the crux of the PR, this is where we're fixing the destination path.

@@ -36,41 +34,17 @@ func TestPack(t *testing.T) {
"Creating .tar.gz file...",
},
expectedFiles: [][]string{
{"pkg", "mypackagename", "bin", "main.wasm"},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ℹ️ Now we're cleaning up after ourselves we need to fix the tests to not check for that directory/files any more.

},
},
// The following test validates that the expected directory structure was
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ℹ️ This test is redundant considering we no longer generate a package using the name field from the fastly.toml manifest file, and so validating that the code works when a name field has a space in the value is unnecessary.

@Integralist Integralist requested a review from shawnps October 13, 2022 08:15
@Integralist Integralist merged commit a08601b into main Oct 17, 2022
@Integralist Integralist deleted the integralist/pack branch October 17, 2022 08:15
Integralist added a commit that referenced this pull request Oct 17, 2022
* fix pack command to generate generic package.tar.gz file name

* clean up the pkg directory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants