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

DVC file -> .dvc file (2nd chunk) reopen #1408

Merged
merged 40 commits into from
Jun 12, 2020

Conversation

utkarshsingh99
Copy link
Contributor

@utkarshsingh99 utkarshsingh99 commented Jun 8, 2020

Partially address #1366
Continuation of #1372
Final version of draft PR #1403

Contains review changes suggested by @jorgeorpinel in #1403

UPDATE: jump to: #1408 (comment)

@calibre-analytics
Copy link

Your trial has ended

Choose a plan to resume monitoring your Sites and Pull Requests. If you need help, check the Manage Your Plan and Test Usage guide.

@utkarshsingh99

This comment has been minimized.

@jorgeorpinel

This comment has been minimized.

@utkarshsingh99

This comment has been minimized.

@utkarshsingh99

This comment has been minimized.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-dvc-file-2-nl7s1r2 June 11, 2020 15:51 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-dvc-file-2-nl7s1r2 June 11, 2020 15:51 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-dvc-file-2-nl7s1r2 June 12, 2020 03:16 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-dvc-file-2-nl7s1r2 June 12, 2020 03:30 Inactive
@utkarshsingh99
Copy link
Contributor Author

@jorgeorpinel Interestingly, the status page is not crashing anymore. I'm still not sure what the problem was, though. 😬

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.

As long as status is working now, it doesn't matter what the problem was 😬

Thanks again for this big one @utkarshsingh99 you have really helped us a lot! I'm approving this one but there's a couple things missing (below) which I will probably just commit myself.

@shcheklein may want to take a quick look before we merge this though.

content/docs/command-reference/status.md Outdated Show resolved Hide resolved
content/docs/command-reference/status.md Outdated Show resolved Hide resolved
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-dvc-file-2-nl7s1r2 June 12, 2020 04:15 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-dvc-file-2-nl7s1r2 June 12, 2020 04:15 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-dvc-file-2-nl7s1r2 June 12, 2020 04:16 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-dvc-file-2-nl7s1r2 June 12, 2020 04:16 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-dvc-file-2-nl7s1r2 June 12, 2020 04:18 Inactive
@jorgeorpinel jorgeorpinel had a problem deploying to dvc-landing-dvc-file-2-nl7s1r2 June 12, 2020 04:18 Failure
@shcheklein shcheklein merged commit 55ba0a1 into iterative:master Jun 12, 2020
@shcheklein
Copy link
Member

@utkarshsingh99 @jorgeorpinel approved and merged but we should go back to whiteboard:

  1. Mentioning dvc.yaml and .dvc is too noisy
  2. Making them links manually in so many places - even more noise, hard to maintain, etc
  3. Some phrases have uncertain meaning/need clarifications because of this form, e.g. A [dvc.yaml](/doc/user-guide/dvc-file-format) file dependency ..

@shcheklein
Copy link
Member

@jorgeorpinel This branch had an error being deployed - also deployment failed? please check the producion, I merged before I checked this

@jorgeorpinel
Copy link
Contributor

Shoot... I will check this. The review ap was working fine last time I checked.

  1. Mentioning dvc.yaml and .dvc is too noisy

Totally agree. I just wanted to get most of the main docs updated as a first step. I think the solution will be to replace all these 2ble mentions with some other concept like probably just "stage" which links to basic concepts and explains it can be either in a dvc.file or a .dvc file (orphan/import stage)

  1. Making them links manually in so many places

Also agree. I think we could make them automatic like command names, at least for dvc.yaml. .dvc file some times we want to link as a whole, sometimes just the first part (.dvc), sometimes not linked. It's tricky because there's also the .dvc/ directory... So again, reducing the number of mentions would be ideal with a proxy concept such as "stage", "orphan stage", "import stage"...

  1. Not sure I understood this point. Perhaps you could list those phrases if it's just a few or somo other examples at least? "A dvc.yaml file dependency" doesn't sound so bad to me (but could be clearer, yes — again by replacing with "stage").

@jorgeorpinel
Copy link
Contributor

This branch had an error being deployed

So the review app doesn't exist anymore unfort. Heroku deletes everything at merge (https://dashboard.heroku.com/apps/dvc-landing-dvc-file-2-nl7s1r2)

I see no build problems in our prod app https://dashboard.heroku.com/apps/dvc-org/activity @shcheklein

@jorgeorpinel
Copy link
Contributor

For 1. and 2. I created #1431

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