-
Notifications
You must be signed in to change notification settings - Fork 23
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
REALMC-10362: Update CLI to handle data_sources dirs with just rules or just schemas/relationships #267
Conversation
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.
initial batch of thoughts/comments from me.
my main concern is I think I'd have expected these changes to be more "additive" than they currently are, and I want to make sure we're not breaking the CLI for any older apps out there
b1578f5
to
92457a0
Compare
92457a0
to
c2717e2
Compare
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.
lgtm!
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.
LGTM 😎 just a couple questions and nits from me!
|
||
rulePath := filepath.Join(collPath, FileRules.String()) | ||
if _, err := os.Stat(rulePath); err != 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.
[opt] could we use Stat()
to check the file paths to see if the conditions for a valid data source folder are met before parsing them? up to you, I'm not sure how much of a performance improvement this would be
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 parseJSON
call below (L#305) actually starts with an os.Stat
call and returns early if the path doesn't exist, so your optimization is already built in here
i think the existing code just took the route of calling os.Stat
ahead of parseJSON
just to be clear about the intent for skipping directories without rules.json
files present (rather than rely on a rule == nil
check)
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.
just a last few questions from me here before this is ready to go... nice work!
|
||
rulePath := filepath.Join(collPath, FileRules.String()) | ||
if _, err := os.Stat(rulePath); err != 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 parseJSON
call below (L#305) actually starts with an os.Stat
call and returns early if the path doesn't exist, so your optimization is already built in here
i think the existing code just took the route of calling os.Stat
ahead of parseJSON
just to be clear about the intent for skipping directories without rules.json
files present (rather than rely on a rule == nil
check)
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.
one last question about our ability within writeDataSources
to not have to write a rules.json
file (which at least currently, shouldn't come up)
otherwise, nothing blocking on this from me, nice work!
🆒 beans
internal/local/structure_v2.go
Outdated
return err | ||
|
||
schema := rule[NameSchema] | ||
if schema != 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.
let's use the if schema, ok := rule[NameSchema]; ok {
syntax here (we avoided it previously b/c we always wanted to replace nil
with map[string]interface{}{}
which i suppose is no longer the case)
internal/local/structure_v2.go
Outdated
|
||
schema := rule[NameSchema] | ||
if schema != nil { | ||
dataSchema, err := MarshalJSON(schema) |
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.
[meganit/naming] could in theory tighten the scope of this named var
to just data
now that we're in this okSchema
closure
…or just schemas/relationships (10gen#267)
JIRA: https://jira.mongodb.org/browse/REALMC-10362
Essentially doing a similar thing to this PR. After this PR, pushing, pulling, and diffing an app should work in cases where:
data_sources/<data_source>/<database>/<collection>
) contains solely arules.json
fileschema.json
andrelationships.json
filesThis change was necessary because, as part of Schema Separation, an app is allowed to have a rule without an associated schema and vice versa