-
Notifications
You must be signed in to change notification settings - Fork 60
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
breaking(compute/pack): use package name from manifest #1025
base: main
Are you sure you want to change the base?
Conversation
pkg/commands/compute/pack.go
Outdated
@@ -44,8 +45,13 @@ func (c *PackCommand) Exec(_ io.Reader, out io.Writer) (err error) { | |||
return err | |||
} | |||
|
|||
filename := sanitize.BaseName(c.manifest.File.Name) |
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.
Note: I think this is a breaking change as it changes what the default output destination is for this command
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.
I would argue that because the existing behaviour doesn't work with fastly compute deploy
(†) that this isn't a breaking change and is fixing broken behaviour.
† The reporter of the issue, an internal report, identified that the
compute deploy
command looks for the package to be named the same as thename
field from the manifest (otherwise it returns an error).
@joeshaw would you concur or do you think this should be a new major release?
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.
I don't have a strong opinion. To what extent do we consider the directory and package names to be exported API? Do external tools consume this that we're aware of? That's the thing that'd be the determining factor to me.
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.
Terraform configurations will need to be updated if they are relying on the default name -- https://github.com/JakeChampion/polyfill-service/blob/main/fastly/terraform/modules/service-compute/main.tf#L7
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.
OK so that's a fair point from @JakeChampion
We have no way to tell if there are Terraform users who have used the compute pack
command to create a custom language Wasm binary (and clearly we do have users taking that approach, as Jake explained: the polyfill.io service is one of them) and so we'd have to publish a new major release if we want this CLI fix rolled out.
My concern is that this is a bug fix for the CLI, but we're inadvertently causing a breaking feature change for Terraform, and yet we need to roll out this bug fix for the CLI still. So avoiding a new major version release isn't possible in this case (correct me if I'm wrong and there are other ways to make this work).
I was going to say if we need to roll out a new major release, then at least we have a couple of other 'breaking' items in the pipeline that can be included in that release.
The only problem with that idea, is that both those breaking items aren't ready yet to be released (not any time soon at least).
Meaning: this PR would either have to wait until that next major release (which could be a long time) or we have to make two major releases (one now to fix the bug in the CLI, and another one later on down the line for these other items).
I'm going to chat internally with @pabergman about this.
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.
OK. I've spoken to Alexander and it sounds like this PR should be held off until we're ready to release a new major release (so I've marked this PR now as a 'breaking' change).
For @justinliew there is a workaround though. The compute deploy
command provides a --package
flag (docs) so that should let you point to a package generated by compute pack
(e.g. compute deploy --package ./pkg/package.tar.gz
).
68fd062
to
062830a
Compare
19f6b0b
to
a93fdbe
Compare
794bc97
to
c423a7e
Compare
ce6b9cc
to
a6d52a8
Compare
6de0e40
to
7caf5e7
Compare
9eb4f6a
to
e08bc8b
Compare
e08bc8b
to
b4ea655
Compare
The
fastly compute deploy
command expects the package file to be named the same as thename
attribute in the fastly.toml manifest. Thefastly compute pack
command does not do this, it instead names the package filepackage.tar.gz
and consequently this causes the deploy command to fail.