Skip to content
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

Merged
merged 6 commits into from
Feb 21, 2020

Conversation

pmrowla
Copy link
Contributor

@pmrowla pmrowla commented Feb 18, 2020

You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ 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

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a 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.

@pmrowla
Copy link
Contributor Author

pmrowla commented Feb 19, 2020

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.

@shcheklein
Copy link
Member

@pmrowla an alternative is to run yarn format-all (or something like this - check the package.json please).

@pmrowla
Copy link
Contributor Author

pmrowla commented Feb 19, 2020

Yeah, I found where the pre-commit hook is specified in package.json and added it to my .git/hooks manually.

@shcheklein
Copy link
Member

@pmrowla sounds good! it interesting though why husky didn't install it after running yarn 🤔 not a big deal though

@pmrowla
Copy link
Contributor Author

pmrowla commented Feb 19, 2020

One thing to note, I took the original docs for password/ask_password from the SSH remote docs, so the usage/clarification issues noted by @shcheklein could also probably be cleaned up there as well.

Copy link
Member

@shcheklein shcheklein left a 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?

@shcheklein
Copy link
Member

@pmrowla please add http://user@example.com/path to link checker exclusion list (see CircleCi complaints)

@jorgeorpinel could you please take a brief look, suggest some final edits here?

@pmrowla pmrowla changed the title [WIP] remote: Document HTTP remote push changes remote: Document HTTP remote push changes Feb 20, 2020
- add http://user@example.com/path
@jorgeorpinel
Copy link
Contributor

Of course, but I think we need to wait for iterative/dvc/pull/3343 to merge before finalizing this.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a 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

public/static/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
public/static/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
public/static/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
public/static/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
public/static/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
public/static/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
public/static/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
public/static/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
public/static/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
public/static/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a 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!

@pmrowla
Copy link
Contributor Author

pmrowla commented Feb 21, 2020

iterative/dvc/pull/3343 has been merged, there were no changes in the source PR since the docs changes were last approved

@jorgeorpinel
Copy link
Contributor

Thank you so much for keeping docs up to date!

@pmrowla pmrowla deleted the http-push branch November 29, 2022 10:27
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.

3 participants