-
Notifications
You must be signed in to change notification settings - Fork 173
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
adding Gitlab support #174
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.
Thanks for the PR @valx ! Left a few comments, would you mind also generating a couple of Stacks projects from this PR and linking them in the PR? Would be good to verify this works with GitLab as expected, and the docs all look ok, as well as other providers remain unaffected.
This comment was marked as resolved.
This comment was marked as resolved.
@valx any update on this PR? Looks like the tests are still failing |
Co-authored-by: Arpit Jasapara <[email protected]>
- for markdown checker of links in .md files
This comment was marked as resolved.
This comment was marked as resolved.
@@ -27,6 +27,7 @@ include: | |||
# Deployment Target specific values for workspace | |||
targets: | |||
dev: {{ if (eq .input_include_models_in_unity_catalog `yes`)}} # UC Catalog Name {{ end }} | |||
mode: development |
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.
@arpitjasa-db Was this intentional?
We have an integration test that confirms successful template initialization and noticed that the job names now are prefixed with the username (DAB default for mode: development
), so checking.
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 intentionally added it as I thought it would be a best practice and most environment I've been working with would benefit from it (as multiple developers could be deploying the same project into dev).
But if that has any negative impact the change could be reverted
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.
It is, no need to revert! It seemed like a piggybacked change, so want to make sure there isn't any docs that need to be updated, or otherwise other downstream changes that need to be made.
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.
Yup intentional! Didn't realize this would break the naming downstream, thanks for flagging. For docs though, we don't really mention the file names for the dev target so we should be good on that
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.
Thanks for confirming.
## Changes The ML production team modified mlops-stack to use `mode: development` for their development target here: databricks/mlops-stacks#174 This PR makes the integration test assertion agnostic of the prefix to make it pass again. ## Tests The test passes now
## Changes The ML production team modified mlops-stack to use `mode: development` for their development target here: databricks/mlops-stacks#174 This PR makes the integration test assertion agnostic of the prefix to make it pass again. ## Tests The test passes now
Main changes:
template/{{.input_root_dir}}/.gitlab/
. Changes include:/mlops-stacks/databricks_template_schema.json
prompt metadata by adding gitlab optioninput_cicd_platform
and added a gitlab optionmode: development
to dev target