Skip to content

Conversation

@macmv
Copy link
Contributor

@macmv macmv commented Sep 10, 2024

Ticket(s): ENG-6705

Adds fauna schema push --stage. This doesn't hit the right endpoint for the non-force version quite yet (as that endpoint doesn't exist), but everything else is in place to push a staged schema.

Base automatically changed from disable-linter-local to main September 10, 2024 19:30
@macmv macmv force-pushed the add-schema-push-staged branch from b3044e6 to efae39e Compare September 10, 2024 22:59
Copy link
Member

@freels freels left a comment

Choose a reason for hiding this comment

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

LGTM, would be good to get a DX team review

Comment on lines +55 to +60
// Confirm diff, then push it. `force` is set on `validate` so we don't
// need to pass the last known schema version through.
const params = new URLSearchParams({
force: "true",
});
const path = new URL(`/schema/1/validate?${params}`, url);
Copy link
Member

Choose a reason for hiding this comment

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

should we just drop the "force" param for validate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you can pass a version instead, so we just require force or version to be set on all the schema endpoints. we could probably drop it, it just keeps the validation simple in the core endpoint.

Copy link
Member

Choose a reason for hiding this comment

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

right, just thinking it might make using the API itself simpler. I forget/lost track if we're actually passing version to validate at any point

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 need to double check, I think we might be using it in the dashboard to check if a schema version is stale. If we're not using it there, I'll go ahead and remove it from the core endpoint.

@echo-bravo-yahoo
Copy link
Contributor

Looks like CI is failing on linting:

/home/circleci/project/src/lib/config/index.ts
  196:14  warning  Constructor has a complexity of 23. Maximum allowed is 20  complexity

/home/circleci/project/src/lib/fauna-import-writer.js
  29:1  warning  Function 'getFaunaImportWriter' has too many parameters (6). Maximum allowed is 4  max-params

@macmv
Copy link
Contributor Author

macmv commented Sep 11, 2024

oh that's what its failing on! i couldn't figure that out for the life of me, all the tests looked fine

well wait no, i haven't touched that code. that should be fine.

@macmv macmv force-pushed the add-schema-push-staged branch 2 times, most recently from 4ae4766 to 78cb6f1 Compare September 11, 2024 23:16
status: "complete",
});

// But, the index should not be visible on the companion object.
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for comments with intent

@macmv macmv force-pushed the add-schema-push-staged branch 3 times, most recently from 7758c38 to a4267b8 Compare September 11, 2024 23:28
@macmv
Copy link
Contributor Author

macmv commented Sep 11, 2024

ok, tests are now failing because the container is a bit out of date. they should start passing next week.

@macmv macmv force-pushed the add-schema-push-staged branch 3 times, most recently from 6126450 to d66c2ca Compare September 16, 2024 23:36
@macmv macmv changed the base branch from main to nevermind-circle-ci-doesnt-have-outages September 16, 2024 23:36
Base automatically changed from nevermind-circle-ci-doesnt-have-outages to main September 17, 2024 21:32
@macmv macmv force-pushed the add-schema-push-staged branch from d66c2ca to 4afab33 Compare September 17, 2024 21:33
@macmv macmv merged commit d0d521e into main Sep 17, 2024
@macmv macmv deleted the add-schema-push-staged branch September 17, 2024 21:33
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.

5 participants