-
Notifications
You must be signed in to change notification settings - Fork 392
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
remote: Document HTTP remote push changes #1006
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.
Hi @pmrowla. Thanks for this! Looks great so far but please ping us again (you can request my review for example) once iterative/dvc/pull/3343 is approved.
The CI checks are failing as of now though. yarn format-check
specifically. Please run yarn
so you have the pre-commit hook that checks/fixes this before committing. See https://dvc.org/doc/user-guide/contributing/docs#development-environment for more info.
I had run yarn, but it looks like the pre-commit hook wasn't installed for some reason? On my local machine I do have git configured to install some pre-commit hooks by default for every repo that I clone, so maybe that's why. It was not an issue when I cloned the dvc source repo, the source pre-commit hook was installed properly there. |
@pmrowla an alternative is to run |
Yeah, I found where the pre-commit hook is specified in package.json and added it to my .git/hooks manually. |
@pmrowla sounds good! it interesting though why husky didn't install it after running yarn 🤔 not a big deal though |
One thing to note, I took the original docs for |
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.
is it still WIP?
@pmrowla please add @jorgeorpinel could you please take a brief look, suggest some final edits here? |
- add http://user@example.com/path
Of course, but I think we need to wait for iterative/dvc/pull/3343 to merge before finalizing this. |
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.
A bunch of language suggestions. Some more relevant than others. Thanks
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 actually approve this. But please let us know when iterative/dvc/pull/3343 is merged, to check whether there are any further changes there that need to be reflected here. Thanks @pmrowla!
iterative/dvc/pull/3343 has been merged, there were no changes in the source PR since the docs changes were last approved |
Thank you so much for keeping docs up to date! |
❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.
🐛 Please make sure to mention
Fix #issue
(if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.Please chose to allow us to edit your branch when creating the PR.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏
Documentation changes for iterative/dvc#3343