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

Fix documentation after code changes #516

Merged
merged 2 commits into from
Jul 29, 2019
Merged

Fix documentation after code changes #516

merged 2 commits into from
Jul 29, 2019

Conversation

alenkacz
Copy link
Contributor

@alenkacz alenkacz commented Jul 8, 2019

No description provided.

@alenkacz alenkacz requested review from kensipe and gerred July 8, 2019 13:03
Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a couple of issues with this change

  1. there is another PR is collides with... if we need a change we should just suggest it there
  2. it is misleading IMO to refer to pkg/kudoctl/bundle/testdata/zk or it's previous content... this is testdata and not for user examples IMO.

@alenkacz
Copy link
Contributor Author

alenkacz commented Jul 8, 2019

@kensipe I agree with 1, I think no docs changes should be merged until we resolve that.

I don't agree with 2. I find it pretty nice that there is a working example that one can run from root of the project. It could have been totally made up file path or one that actually means something in the project. I went the second way. If we don't like it, we can still pretend it's made up path :) I just changed it...

@kensipe
Copy link
Member

kensipe commented Jul 8, 2019

Doesn't it make more sense to use a samples folder for user facing samples tho... instead of test/data?

Copy link
Member

@fabianbaier fabianbaier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@alenkacz
Copy link
Contributor Author

alenkacz commented Jul 10, 2019

@kensipe I see your point. My reasoning was that this way it's less yaml to maintain, plus you know this yaml works because we always exercise it in tests. It's like the jsons we have in marathon - we use them in tests and they are good documentation at the same time. I was looking at this in the same way

I agree that the location of those samples is not very straightforward :( but I hope we won't shuffle folders that much anymore

@alenkacz alenkacz added the hold label Jul 10, 2019
@kensipe kensipe merged commit c27e6f7 into master Jul 29, 2019
@alenkacz alenkacz deleted the av/fix-install-docs branch October 30, 2019 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants