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

feat(generateScratchOrgInfo): allow shema key #1113

Closed

Conversation

alanjaouen
Copy link
Contributor

@alanjaouen alanjaouen commented Jul 29, 2024

What does this PR do?

This pr remove $schema key from parsed scratchorg definition. allowing to pass check scratchOrgInfoPayload must be heads down camelcase. and so preventing InvalidJsonCasing exceptions

What issues does this PR fix or reference?

InvalidJsonCasing are raised if definition file contains $schema key (from documentation)

@alanjaouen alanjaouen requested a review from a team as a code owner July 29, 2024 09:50
@mshanemc
Copy link
Contributor

mshanemc commented Aug 7, 2024

Cool, thanks for noticing and fixing.

Are you willing/able to add a unit test for that? It should probably go in this describe block

describe('generateScratchOrgInfo', () => {
const testData = new MockTestOrgData();

(pass it an object with your $schema property and verify that it's removed.

Your code looks fine and I'm sure it works, but the test will preserve the behavior through future refactorings.

Copy link

git2gus bot commented Aug 7, 2024

This issue has been linked to a new work item: W-16441182

@alanjaouen alanjaouen force-pushed the feat/allow-schama-sodef branch from acf0c55 to 4a5fdd9 Compare August 7, 2024 22:33
@alanjaouen
Copy link
Contributor Author

Thanks for feedback

I added a test for this, let me know if there is a better way to make this test

@mshanemc
Copy link
Contributor

mshanemc commented Aug 8, 2024

Looks great, thanks for doing that.

I reorganized the tests a bit and moved it to a branch on our repo so all the integration tests can run.
#1124

@mshanemc mshanemc closed this Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants