-
Notifications
You must be signed in to change notification settings - Fork 70
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 environment migration #2534
Conversation
server/model/environments.go
Outdated
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.
Instead of having an Environment2
, can we kill this model? By doing that we are able to see if we migrated all the env stuff too. Does it make sense?
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.
Forgot to delete it haha
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.
Left a few suggestions, but this looks great! love how much code you killed
cli/formatters/environment.go
Outdated
@@ -56,11 +59,19 @@ func (f EnvironmentsFormatter) ToStruct(file *file.File) (interface{}, error) { | |||
var environmentResource openapi.EnvironmentResource | |||
nullableEnvironment := openapi.NewNullableEnvironmentResource(&environmentResource) | |||
|
|||
if strings.HasSuffix(file.Path(), ".yaml") || strings.HasSuffix(file.Path(), ".yml") { |
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.
Do we need this validation? Maybe we could try to parse the file, assuming that file definitions must be yaml, and throw an error if it's not parseable. Does that make sense?
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.
is it guaranteed that the file will always be yaml?
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.
A solution (I don't like it much) is to try to parse it as YAML, and then as JSON as a fallback:
func (f EnvironmentsFormatter) ToStruct(file *file.File) (interface{}, error) {
var environmentResource openapi.EnvironmentResource
nullableEnvironment := openapi.NewNullableEnvironmentResource(&environmentResource)
if err := yaml.Unmarshal([]byte(file.Contents()), &environmentResource); err == nil {
return environmentResource, nil
}
err := nullableEnvironment.UnmarshalJSON([]byte(file.Contents()))
if err != nil {
return nil, err
}
return environmentResource, nil
}
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.
the original implementation fallbacks to json, and that is also not guaranteed. I think in the end its a product decision: do we want to support files in formats other than YAML?
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.
We could assume that it will be YAML and return an error message in case of a parsing error, to avoid having too much parsing logic or guessing on the CLI. Does it make sense?
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'm considering the file ContentType. It's hardcoded as YAML. But I still left the json fallback there just in case we change that in the future
server/http/controller.go
Outdated
testDB model.Repository | ||
runner runner | ||
newTraceDBFn func(ds datastoreresource.DataStore) (tracedb.TraceDB, error) | ||
environmentRepository environment.Repository |
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.
you are only using the Get
function, so maybe instead of depending on the repo directly, you could depend on a local interface:
type envGetter interface {
Get(context.Context, id.ID) (environment.Environment, error)
}
type controller struct {
//...
environmentRepository envGetter
}
that way if we need to pass a mock or any other substitution, we only need to implement that func and not the entire repo
@@ -22,6 +23,16 @@ type ( | |||
) | |||
|
|||
func (e Environment) Validate() error { | |||
if e.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.
❤️ validation being used
server/http/controller_test.go
Outdated
@@ -120,6 +121,7 @@ func setupController(t *testing.T) controllerFixture { | |||
nil, | |||
nil, | |||
mappings.New(traces.NewConversionConfig(), comparator.DefaultRegistry(), mdb), | |||
environment.NewRepository(nil), |
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.
referecing my previous comment, this could also be just nil
instead of an invalid repo, which reads nicer I think (personal opinion)
@@ -32,15 +32,6 @@ type RunRepository interface { | |||
GetLatestRunByTestVersion(context.Context, id.ID, int) (Run, error) | |||
} | |||
|
|||
type EnvironmentRepository interface { |
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.
❤️
* migrate environment usage to use new resource structure * fix env apply and add test to server env model * fix stuff * fix tests * delete old environment struct * PR suggestions * remove json fallback * use pointer to repository instead of passing by value * fix test
This PR finishes migrating the environment from the old api endpoints to the new resource format.
Checklist