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

Improve Docs (focusing on local development) #1771

Merged
merged 12 commits into from Mar 1, 2019

Conversation

@dimitropoulos
Copy link
Contributor

dimitropoulos commented Feb 27, 2019

This is a draft pull request for some miscellaneous blips and bloops found while getting started with local development.

site/how-it-works.md Outdated Show resolved Hide resolved
@dimitropoulos dimitropoulos force-pushed the dimitropoulos:improveDocs branch from 8f55aab to 1b4598a Feb 27, 2019
site/helm-get-started.md Outdated Show resolved Hide resolved
site/helm-get-started.md Outdated Show resolved Hide resolved
@dimitropoulos dimitropoulos force-pushed the dimitropoulos:improveDocs branch from 1c3354c to 1b4598a Feb 27, 2019
@dimitropoulos

This comment has been minimized.

Copy link
Contributor Author

dimitropoulos commented Feb 27, 2019

@stefanprodan thanks for the approval! it's one of those new/fancy draft pull requests, meaning I'm going to keep it open for a little bit (as a work-in-progress PR might) and add to it as I find things to add. I suspect that will include some changes to the building file to say a little more about the development workflow.

@rade

This comment has been minimized.

Copy link
Contributor

rade commented Feb 27, 2019

I'm going to keep it open for a little bit

Does the PR as it stands improve on the status quo? If so, merge it!

@dimitropoulos

This comment has been minimized.

Copy link
Contributor Author

dimitropoulos commented Feb 27, 2019

@rade I catch your drift (and the answer is yes) but the thing I'm trying to avoid is to have to go through the branch creation - pull request - review - merge cycle for every individual commit (not that you're suggesting to go that granular, but in fairness, there's almost nothing of substance yet in this PR). I expect to have this merged by the end of the week and I don't want to spend time creating pull requests when I could spend that same time coding. if the trade-off of that is such that it will take a few extra days for these things to land on master then I think that's an acceptable trade-off.

@dimitropoulos

This comment has been minimized.

Copy link
Contributor Author

dimitropoulos commented Feb 27, 2019

but more generally @stefanprodan @squaremo if you would like me to merge it, just let me know and I'm happy to push up another PR at the end of the week. I'm actually surprised to find that it's possible to review (or approve) a draft PR before I've signaled that it's "Ready for review". here is what it looks like on my end:

screenshot_20190227_162020

Copy link
Contributor

dholbach left a comment

Thanks for your work on this! A small suggestion, I'll leave it up to you if you want to make the change.

site/how-it-works.md Outdated Show resolved Hide resolved
@dimitropoulos dimitropoulos force-pushed the dimitropoulos:improveDocs branch from a86167e to 3b2c814 Feb 28, 2019

1. The [Minikube addon](https://github.com/kubernetes/minikube/blob/master/docs/addons.md) called [freshpod](https://github.com/GoogleCloudPlatform/freshpod) that will be very useful to us later. You'll see. It's gonna be cool.
```sh
minikube addons enable freshpod

This comment has been minimized.

Copy link
@stefanprodan

stefanprodan Feb 28, 2019

Member

Can you please replace Freshpod with Skaffold, Freshpod is no longer maintained.

This comment has been minimized.

Copy link
@dimitropoulos

dimitropoulos Feb 28, 2019

Author Contributor

Yes. @squaremo had suggested I used freshpod. That, and it being super easy (with the one-liner) to integrate was the reason for using it.


## Run `flux-getting-started`

1. We're going to make some changes soon enough, but just to get a good baseline please follow the [Getting Started](site/get-started.md) guide and run the `flux-getting-started` repo through its normal paces.

This comment has been minimized.

Copy link
@dholbach

dholbach Mar 1, 2019

Contributor

Minor: Probably doesn't need the 1. as it's just one step.

Copy link
Contributor

dholbach left a comment

[ This is about site/get-started-developing.md ... ]

Thanks @dimitropoulos for your work on this. It reads well, goes into detail, but not into too much detail.

The last paragraph under "Make Some Changes" is written a bit colloquially ("the Kubernetes api server [..] will go ..."), which is different in style from any other of the flux docs, but I feel that's fine.

As I'm not regularly hacking on Flux, I'd recommend to also ask @squaremo @hiddeco @2opremio or @stefanprodan for input. Maybe we can even agree on the tools we use for development? :-D

@dimitropoulos

This comment has been minimized.

Copy link
Contributor Author

dimitropoulos commented Mar 1, 2019

Maybe we can even agree on the tools we use for development? :-D

That is the current issue. @stefanprodan has rightly pointed out that it's not super-duper responsible to put freshpod in here since it's on its way to the grave (with skaffold as the official successor). The problem is that it's very much still supported by minikube and does what it does well at the same time as being stupidly-simple to configure (just minikube addons enable freshpod and that's it). All the same, I'm trying my hand at getting skaffold going in place of freshpod.

@rade

This comment has been minimized.

Copy link
Contributor

rade commented Mar 1, 2019

freshpod vs skaffold

I wouldn't hold up this PR for that. Go with freshpod and change the recommendation to skaffold once that has been shown to work well.

@dimitropoulos

This comment has been minimized.

Copy link
Contributor Author

dimitropoulos commented Mar 1, 2019

ok. will do.

@dimitropoulos dimitropoulos force-pushed the dimitropoulos:improveDocs branch from 6302a6a to 17262ec Mar 1, 2019
for those bean-counters in the audience I timed it: this change took 2m21s
among many many other reasons since this is a markdown file and to reasonably view a raw markdown file you must use word wrapping, I hope the awkwardness of the previous commit well demonstrates why this change greatly simplifies the document.
Don't worry, I'm a certified vim ninja - many of these changes were like 4 keystrokes.
(⌐■_■)
@dimitropoulos dimitropoulos marked this pull request as ready for review Mar 1, 2019
@dimitropoulos dimitropoulos merged commit 22a7d3e into fluxcd:master Mar 1, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@dimitropoulos dimitropoulos deleted the dimitropoulos:improveDocs branch Mar 1, 2019
@squaremo

This comment has been minimized.

Copy link
Member

squaremo commented Mar 4, 2019

Thanks for sticking at this @dimitropoulos 🍱

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.