-
Notifications
You must be signed in to change notification settings - Fork 59
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(service-version): Allow 'locked' services to be activated. #1245
Conversation
6048ddb
to
89cdc81
Compare
It looks like a third-party dependency has been manually copied into the repo? From what I can tell the original code came from https://github.com/leighmcculloch/go-optional We should be adding this as a dependency to the CLI e.g. |
The new package has now been added to |
Unfortunately |
I'll take a look on Monday and see what we can do |
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.
Other than the issue with go mod tidy which I'll look at Monday, this LGTM
Yeah I think we'll just have to use the import path used in that optional project... diff --git a/go.mod b/go.mod
index b1c9b1bc..aaed6c53 100644
--- a/go.mod
+++ b/go.mod
@@ -79,8 +79,4 @@ require (
gopkg.in/yaml.v3 v3.0.1 // indirect
)
-require (
- github.com/leighmcculloch/go-optional v0.2.0
-)
-
-replace github.com/leighmcculloch/go-optional => 4d63.com/optional v0.2.0
+require 4d63.com/optional v0.2.0
diff --git a/pkg/argparser/cmd.go b/pkg/argparser/cmd.go
index 50e9ce62..3be4104b 100644
--- a/pkg/argparser/cmd.go
+++ b/pkg/argparser/cmd.go
@@ -13,7 +13,7 @@ import (
"github.com/fastly/cli/pkg/global"
"github.com/fastly/cli/pkg/manifest"
"github.com/fastly/cli/pkg/text"
- optional "github.com/leighmcculloch/go-optional"
+ "4d63.com/optional"
) |
I know there are strong opinions about this topic, and I'm fairly new to the Golang community, but how would you feel about using 'go mod vendor' here? I'm mostly concerned about build reproducibility. Most of our dependencies are heavily-used packages and aren't likely to be a problem, but this one ( |
After discussion with various knowledgeable Go users we've decided to stick with the 'vanity path' used for the |
This PR changes the ServiceDetailsOpts structure (used by the ServiceDetails function) to permit commands to specify individual serviceversion states as part of the filtering mechanism. Nearly all commands allow both 'active' and 'locked' serviceversions, and previously 'service-version activate' blocked both, but it should allow 'locked' while still blocking 'active'. Additionally, the error message emitted when the specified serviceversion is not allowed by the filter now indicates whether it was the 'active' or 'locked' status which caused the error. Finally, many more tests were added to ensure that the proper error messages are emitted for 'active' and 'locked' serviceversions in many of the commands which specify filters.
This PR changes the ServiceDetailsOpts structure (used by the ServiceDetails function) to permit commands to specify individual service states as part of the filtering mechanism. Nearly all commands allow both 'active' and 'locked' services, and previously 'service-version activate' blocked both, but it should allow 'locked' while still blocking 'active'.
Additionally, the error message emitted when the specified service is not allowed by the filter now indicates whether it was the 'active' or 'locked' status which caused the error.
Closes #1191.