Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Improve Docs (focusing on local development) #1771

Merged
merged 12 commits into from
Mar 1, 2019

Conversation

dimitropoulos
Copy link
Contributor

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
site/helm-get-started.md Outdated Show resolved Hide resolved
site/helm-get-started.md Outdated Show resolved Hide resolved
@dimitropoulos
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
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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Member

@dholbach dholbach left a comment

Choose a reason for hiding this comment

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

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@dholbach dholbach left a comment

Choose a reason for hiding this comment

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

[ 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
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
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
Copy link
Contributor Author

ok. will do.

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 March 1, 2019 19:20
@dimitropoulos dimitropoulos merged commit 22a7d3e into fluxcd:master Mar 1, 2019
@dimitropoulos dimitropoulos deleted the improveDocs branch March 1, 2019 19:25
@squaremo
Copy link
Member

squaremo commented Mar 4, 2019

Thanks for sticking at this @dimitropoulos 🍱

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants