Skip to content

Update output for Data Preparation processing#1808

Merged
fernst merged 4 commits intomainfrom
update-datapreparation-encoding
Aug 15, 2024
Merged

Update output for Data Preparation processing#1808
fernst merged 4 commits intomainfrom
update-datapreparation-encoding

Conversation

@fernst
Copy link
Copy Markdown
Collaborator

@fernst fernst commented Aug 13, 2024

Updated data preparation proto to only carry binary encoded data preparation contents after processing the data preparation definition.

@fernst fernst requested review from Ekrekr, chtyim and lewish August 13, 2024 15:41
Comment thread protos/core.proto Outdated
bool disabled = 6;

DataPreparationDefinition data_preparation = 7;
reserved 7;
Copy link
Copy Markdown
Contributor

@Ekrekr Ekrekr Aug 13, 2024

Choose a reason for hiding this comment

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

Reserved fields should be at the end of messages

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Updated

Comment thread core/actions/data_preparation.ts Outdated
Comment on lines 37 to 38
const dataPreparationContents = nativeRequire(config.filename).asJson;
const dataPreparationDefinition = parseDataPreparationDefinitionJson(dataPreparationContents);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: the term dataPreparationContents is overloaded now, so should be renamed - something like dataPreparationAsJson = ...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Updated

":bundle_with_license",
":configs_proto_with_license",
":core_proto_with_license",
":data_preparation_proto_with_license",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You're still going to need this exported, for the decode step

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I added this back as discussed.

@fernst fernst requested a review from Ekrekr August 13, 2024 19:45
Comment thread core/main_test.ts Outdated
}
]
}
dataPreparationContents: "Cn4KBW5vZGUxEhASDgoDcHJqEgJkcxoDc3JjIlAiFgoUCgFhEgVJTlQ2NBoITlVMTEFCTEUqGyIZEhcKFQoBYRIGU1RSSU5HGghOVUxMQUJMRTIZChcKFQoBYRIGU1RSSU5HGghOVUxMQUJMRSoRCg8KA3ByahICZHMaBGRlc3Q=",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is probably generated by some external script? Might be good to include that script / instruction on how to update this string.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good shout - @fernst if you bring back what you put here before, and just call dataform.DataPreparationDefinition.encode( on it in the test part, then its generation becomes clear

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I'll add this logic so it's obvious.

Comment thread core/main_test.ts Outdated
}
]
}
dataPreparationContents: "Cn4KBW5vZGUxEhASDgoDcHJqEgJkcxoDc3JjIlAiFgoUCgFhEgVJTlQ2NBoITlVMTEFCTEUqGyIZEhcKFQoBYRIGU1RSSU5HGghOVUxMQUJMRTIZChcKFQoBYRIGU1RSSU5HGghOVUxMQUJMRSoRCg8KA3ByahICZHMaBGRlc3Q=",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good shout - @fernst if you bring back what you put here before, and just call dataform.DataPreparationDefinition.encode( on it in the test part, then its generation becomes clear

@Ekrekr
Copy link
Copy Markdown
Contributor

Ekrekr commented Aug 14, 2024

Our tests are being flakey atm because of the Invalid JWT Signature error - if you rerun them they should go away

image

@fernst fernst requested a review from chtyim August 14, 2024 15:28
@fernst fernst force-pushed the update-datapreparation-encoding branch from a38ea41 to 698ae58 Compare August 15, 2024 16:06
@fernst fernst merged commit 2531b12 into main Aug 15, 2024
@fernst fernst deleted the update-datapreparation-encoding branch August 15, 2024 16:11
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.

3 participants