-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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(cli): option to init via username password, add lint for smoke-test #9675
Conversation
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.
mostly looks good, just a few comments on code style / docs
thanks for doing this :)
773d048
to
cd49ce4
Compare
3d65afa
to
21c38bd
Compare
bc69b11
to
75adfc9
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.
mostly looks good
with: | ||
distribution: "zulu" | ||
java-version: 17 | ||
- name: Run lint on smoke test |
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 should not be in the setup job, since it'll block the rest of the docker builds
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.
That is actually the intention. Lint should cause rest of CI to simply not run. This causes anyone making changes in smoke tests to fail fast. Otherwise CI will wait for docker builds which takes a while before failing.
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.
I'm not sure I agree with that - this will add a few minutes of wall clock time to every docker-unified run, which is already the slowest part of our CI. Lint issues are going to be relatively rare in smoke test, so I don't think it makes sense to slow down our CI significantly to avoid a small number of wasted docker builds
Instead the lint should be a separate job so that the docker builds can run in parallel, and we can use something like this https://github.com/orgs/community/discussions/38361 to cancel the other workflows
from datahub.cli.env_utils import get_boolean_env_variable | ||
|
||
__help__ = ( | ||
"For helper methods to contain manipulation of the config file in local system." |
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.
module level docstrings should go at the top and be in """
, don't need to use the __help__
thing
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.
Going to do this in follow up PR.
Changes
--use-password
indatahub init
to allow easier generation of tokens for admin for initial setupdatahub init
is run in configuring gms url leading to connection issuessmoke-test
directorysmoke-test
Follow up work todo in separate PR
smoke-test
has__init__
otherwise those tests won't workChecklist