Skip to content
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

Validate workflow settings fields #1581

Merged
merged 6 commits into from
Nov 22, 2023
Merged

Conversation

Ekrekr
Copy link
Contributor

@Ekrekr Ekrekr commented Nov 20, 2023

Include a generic best effort Protobuf field validator.

@Ekrekr Ekrekr requested a review from BenBirt November 20, 2023 17:35
@@ -14,6 +14,33 @@ export interface IProtoClass<IProto, Proto> {
fromObject(obj: { [k: string]: any }): Proto;
}

// ProtobufJS's native verify method does not check that only defined fields are present.
// TODO(ekrekr): swap to Typescript protobuf library rather than having to do this.
export function verifyObjectMatchesProto<Proto>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i hate this, let's please switch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some digging into this, and the alternatives which can validate (https://www.npmjs.com/package/@protobuf-ts/plugin, https://www.npmjs.com/package/protoc-gen-ts) are significantly slower. More details in b/305915462#comment18.

@Ekrekr Ekrekr merged commit 2a8f584 into main_v3 Nov 22, 2023
4 checks passed
@Ekrekr Ekrekr deleted the workflow-settings-validation branch November 22, 2023 11:50
moker-spaghetti pushed a commit to moker-spaghetti/dataform that referenced this pull request May 26, 2024
* Add workflow settings validation

* Tidy

* Remove return from recursive field check

* Fix object lint error

* Remove braking run cache option from CLI

* Remove console log
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants