-
Notifications
You must be signed in to change notification settings - Fork 542
feat: ensure endgame task comp registration + complete metadata bundle #3949
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
feat: ensure endgame task comp registration + complete metadata bundle #3949
Conversation
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
bindings/gcp/pubsub/pubsub.go
Outdated
| ProjectID string `json:"projectID"` | ||
| PrivateKeyID string `json:"privateKeyID"` | ||
| PrivateKey string `json:"privateKey"` | ||
| ClientEmail string `json:"clientEmail"` | ||
| ClientID string `json:"clientID"` | ||
| AuthURI string `json:"authURI"` | ||
| TokenURI string `json:"tokenURI"` | ||
| AuthProviderX509CertURL string `json:"authProviderX509CertURL"` | ||
| ClientX509CertURL string `json:"clientX509CertURL"` |
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.
pls note that i did update these intentionally. This is specific to bindings gcp pubsub which is not stable or listed as any component type from what I saw. I updated this to match the existing stable gcp pubsub component type and so that way i can use the builtin gcp auth provider properly.
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
…nalyzer happy Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
|
I wholly support this PR in its effort to ensure that the components-metadata-bundle.json file reflects accurate descriptions of what each of the supported components expect with all the schema validation. Further, if this is something we can represent as a script that keeps these metadata outputs consistently up to date as part of the repository build/release pipeline, that would be fantastic for ensuring component parity with each tagged release! |
Signed-off-by: Samantha Coyle <[email protected]>
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 are a large number of changes in this PR spanning multiple groups and concepts which makes it tricky to review. I this makes it higher risk that something is added which is incorrect or breaks something.
It has been a long standing issue that the metadata.yaml files should be generated, however I think this should come from a generi code type reflector which appropriate comment tag decoration of the input metadata.
As it stands, I don't want to move forward this all these additions where we rely on humans creating these files manually. Please can we at least split this PR into multiple PRs where each target a different change/metadata.yaml addition.
| - name: secret | ||
| required: false | ||
| description: "The webhook secret for signature verification" | ||
| example: '"your-webhook-secret"' No newline at end of file |
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.
All files should include a new line at EOF
bindings/gcp/pubsub/pubsub.go
Outdated
| func (g *GCPPubSub) getPubSubClient(_ context.Context, metadata *pubSubMetadata) (*pubsub.Client, error) { | ||
| var pubsubClient *pubsub.Client | ||
| var err error | ||
|
|
||
| if metadata.PrivateKeyID != "" { | ||
| // TODO: validate that all auth json fields are filled | ||
| authJSON := &GCPAuthJSON{ |
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.
Where has this code come from?
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.
| "github.com/dapr/kit/ptr" | ||
| ) | ||
|
|
||
| // TODO: I think we need more defaults set on these metadata fields. |
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.
Why this TODO?
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 am open to suggestions on how to make this comment clearer.
As an approver in this repo, I take ownership seriously, and I use TODO comments intentionally to highlight areas that need improvement but are outside the current scope of my changes.
In this case, the comment came from a thorough review of each metadata.yaml file I personally touched in this PR. I noticed that many fields — particularly in alpha/beta components — are missing default values that would improve clarity and usability. While addressing that across the board felt out of scope for this specific task, I still wanted to surface it as something worth revisiting.
Happy to reword the comment or track it elsewhere if you have a suggestion
I split this effort out across component types for each PR :) |
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
|
ready for review pls |
Description
For each dapr release there is an endgame task to ensure that components are properly registered across contrib and dapr/dapr. I took a bit of time to create a new CLI with the other CLI cmds under
/build-toolsto automate this for myself (assigned this task), as well as for others. I also took the time to fix all the issues that I came across, cross referencing the docs. I have a list of components that need to be added to the docs. This took longer than one would hope; however, I noticed several components not registered properly in runtime, or in the docs, and several component types with no metadata.yaml files at all. I went ahead and added these metadata.yaml files myself as part of this endgame task. This PR is a first stab at fully automating this endgame task of ensuring component registration (and a complete metadata bundle) and does not hit every check box on my issue I created below, but again, is a great starting point and can be iterated on for future releases.It's also nice bc i print out the counts, so we have accurate metrics in future and can also cross ref in the docs later on which more are missing than I found in my manual cross referencing
I've fixed all issues; however, this is what my cli will output in the case of errs
I also went through all metadata files and ensured we adhere to the types our metadata bundle accepts which are (component-metadata-schema.json):
Issue reference
#3948
fixes
#3946
#3945
#3943
Please reference the issue this PR will close: #[issue number]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
Note: We expect contributors to open a corresponding documentation PR in the dapr/docs repository. As the implementer, you are the best person to document your work! Implementation PRs will not be merged until the documentation PR is opened and ready for review.