-
Notifications
You must be signed in to change notification settings - Fork 117
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
Dedicated connectors #4956
Dedicated connectors #4956
Conversation
# Conflicts: # proto/gen/rill/runtime/v1/resources.pb.go
@@ -499,3 +500,17 @@ message ExecutionError { | |||
message CharLocation { | |||
uint32 line = 1; | |||
} | |||
|
|||
message ConnectorSpec { | |||
string name = 1; |
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.
It doesn't need a name
field here because every resource already has a name (in meta.name.name
)
// Dedicated S3 connector | ||
"/connectors/s3-dedicated.yaml": ` | ||
driver: s3 | ||
name: s3-dedicated |
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 name defaults to the filename without extensions (for all resources), so not necessary to set explicitly
runtime/registry.go
Outdated
|
||
instance, err := r.get(iwc.instanceID) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
p, err := rillv1.Parse(iwc.ctx, repo, iwc.instanceID, instance.Environment, instance.OLAPConnector) |
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.
There's a call to ParseRillYAML
right above that I think is also redundant now that it's anyway doing a full parse. Should be able to just check if rill.yaml
is missing by accessing p
.
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.
Yes, it is redundant. I also simplified a definition of UpdateInstanceWithRillYAML
that is called next
name := tmp.Name | ||
if name == "" { | ||
name = node.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.
Can be removed and just use node.Name
directly. The name inference logic is already applied in parseStem
:
rill/runtime/compilers/rillv1/parse_node.go
Line 252 in b46a80a
// If name is not set in YAML or SQL, infer it from path |
Closes #4967