-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[dg] add check definitions command #27873
base: master
Are you sure you want to change the base?
Conversation
30db274
to
e5ff88a
Compare
ee23a34
to
31f10d3
Compare
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.
This is definitely functionality we want but I am strongly opposed to dg definitions validate
as the name. Explained why in this doc which I posted in the channel and which touches on questions I'd like to settle before merging this: https://www.notion.so/dagster/Proposal-project-deployment-code-location-and-the-command-structure-of-dg-19c18b92e46280ad9d72e54cafdeec8e?pvs=4
Summary of that doc wrt this PR is that I think we should have a top-level check
command and this should be dg check definitions
(alongside dg check yaml
).
Totally agree, I am unhappy with the command name as-written in this PR. Will wait for resolution on that doc. |
9b89cf9
to
2ac626b
Compare
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.
Can land this now as dg check definitions
per discussion.
Summary
Introduces
dg definitions validate
, which calls the underlyingdagster definitions validate
with appropriate parameters for the current deployment or code location project. Largely cribbed fromdg dev
.We could rename this to just
dg validate
ordg definitions-validate
to avoid adding another top-level CLI.There's also a fair amount of logspew on the
dagster definitions validate
end that a stacked PR can clean up.Test Plan
Some brief test cases, tested manually.