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

Exclude Helm manifests in get-started guide #1902

Merged
merged 1 commit into from Apr 9, 2019

Conversation

@2opremio
Copy link
Collaborator

2opremio commented Apr 4, 2019

The get started guide uses repo github.com/weaveworks/flux-get-started
which contains HelmRelease manifests, but the guide didn't instruct
the reader to deploy the Helm operator.

Fixes #1898

@2opremio 2opremio requested review from dholbach and hiddeco Apr 4, 2019
@hiddeco

This comment has been minimized.

Copy link
Member

hiddeco commented Apr 4, 2019

I think we should make it more clear to the user that installation of the Helm operator is optional (and only required because the flux-get-started repository makes use of it).

If the user now follows these steps, but for example, has not installed Tiller on the cluster (because we did not tell them to), the apply of the HelmRelease will succeed but the roll-out of the release itself will fail (and the operator will only be logging errors).

@2opremio

This comment has been minimized.

Copy link
Collaborator Author

2opremio commented Apr 4, 2019

That's true. Then, we either change the example repository to one without HelmReleases or we add instructions to install Tiller and indicate that helm is optional. Thoughts?

@2opremio

This comment has been minimized.

Copy link
Collaborator Author

2opremio commented Apr 4, 2019

BTW, how could the guide have possiblly ever worked? I'll do some digging

@hiddeco

This comment has been minimized.

Copy link
Member

hiddeco commented Apr 4, 2019

From the git log in my head; there was a commit a while ago that just changed the repository URLs without matching the content.


Head was right: #1527

@hiddeco

This comment has been minimized.

Copy link
Member

hiddeco commented Apr 4, 2019

Then, we either change the example repository to one without HelmReleases or we add instructions to install Tiller and indicate that helm is optional.

I think the latter would be good, with a note saying that for demo purposes we install the operator too (and thus Tiller must be installed) but it is fine to only make use of the Flux daemon.

@dholbach

This comment has been minimized.

Copy link
Contributor

dholbach commented Apr 5, 2019

Hum. Maybe I'm missing something, but AFAICS the get started tutorial is about "flux standalone" (cf this note), whereas the helm get started tutorial is about helm: the need to install Tiller is discussed there.

@hiddeco

This comment has been minimized.

Copy link
Member

hiddeco commented Apr 5, 2019

AFAICS the get started tutorial is about "flux standalone"

Correct, the problem is that the repository linked in this doc (and used as an example) contains HelmRelease resources, confusing users (see linked issue).

@2opremio

This comment has been minimized.

Copy link
Collaborator Author

2opremio commented Apr 5, 2019

@dholbach

This comment has been minimized.

Copy link
Contributor

dholbach commented Apr 5, 2019

This is unfortunate. It'd be great if we could

  1. maintain just one example
  2. have a straight-forward example with clear instructions that you can easily tailor to your use-cases

It looks like we can't easily have it all this time.

My inclination would be to merge this PR, but make a note that we at least document why this is necessary in the future, and even better: make the use of the example repo here more "obvious"?

@2opremio

This comment has been minimized.

Copy link
Collaborator Author

2opremio commented Apr 5, 2019

It looks like we can't easily have it all this time.

We can also restrict the repo to directories not including HelmReleases by configuring the flux deployment. I think I will do that.

@2opremio 2opremio force-pushed the 2opremio:helm-op-get-started branch from ac8051d to f451677 Apr 5, 2019
@2opremio 2opremio changed the title Deploy helm operator in get-started guide Exclude Helm manifests in get-started guide Apr 5, 2019
@2opremio

This comment has been minimized.

Copy link
Collaborator Author

2opremio commented Apr 5, 2019

@hiddeco
hiddeco approved these changes Apr 5, 2019
Copy link
Member

hiddeco left a comment

I think we got ourselves a winner 💯

@2opremio 2opremio force-pushed the 2opremio:helm-op-get-started branch from f451677 to 0ca2796 Apr 5, 2019
@dholbach

This comment has been minimized.

Copy link
Contributor

dholbach commented Apr 5, 2019

Thanks for the fix - this looks clean. (Even if it adds another step to the "get started" experience - I guess I'll file a separate issue about this.)

Have you tried this locally? In my case the new podinfo gets deployed, but for some reason the color does not change.

@2opremio

This comment has been minimized.

Copy link
Collaborator Author

2opremio commented Apr 5, 2019

In my case the new podinfo gets deployed, but for some reason the color does not change.

It does work for me. Do the logs show anything odd?

I don't see why any of the changes I made would cause flux not to update the deployment correctly.

@dholbach

This comment has been minimized.

Copy link
Contributor

dholbach commented Apr 5, 2019

Just started with a fresh minikube, same thing. The correct revision is deployed. Colour stays green.

@2opremio

This comment has been minimized.

Copy link
Collaborator Author

2opremio commented Apr 8, 2019

I will take a deeper look. However I doubt that's caused by this PR. Does it work when deploying helm and the helm op?

@dholbach

This comment has been minimized.

Copy link
Contributor

dholbach commented Apr 8, 2019

The tutorials follow different paths:

  • standalone get started asks people to change PODINFO_UI_COLOR to blue in workloads/podinfo-dep.yaml - this seems not to work in either of the variants.
  • helm get started asks people to update the image rev of mongodb, this works.
@dholbach

This comment has been minimized.

Copy link
Contributor

dholbach commented Apr 8, 2019

Maybe we can just copy https://github.com/weaveworks/flux/blame/master/site/helm-get-started.md#L147 (147 and following) over to get-started.md?

@dholbach

This comment has been minimized.

Copy link
Contributor

dholbach commented Apr 8, 2019

/cc @stefanprodan on the above

@hiddeco

This comment has been minimized.

Copy link
Member

hiddeco commented Apr 8, 2019

@2opremio

This comment has been minimized.

Copy link
Collaborator Author

2opremio commented Apr 8, 2019

Can me merge this and create a separate issue for the color?

@2opremio

This comment has been minimized.

Copy link
Collaborator Author

2opremio commented Apr 8, 2019

It does work for me.

I retried and it didn't, I would had sworn it did. ¯_(ツ)_/¯

Sorry @dholbach

@dholbach

This comment has been minimized.

Copy link
Contributor

dholbach commented Apr 8, 2019

Let's copy over the Helm Get Started bits into the other tutorial in a separate issues.

@squaremo

This comment has been minimized.

Copy link
Member

squaremo commented Apr 9, 2019

Is this ready to merge or is it blocked on something -- I'm not clear after reading the comments 😅

@dholbach

This comment has been minimized.

Copy link
Contributor

dholbach commented Apr 9, 2019

Yes, let's do it, and fix the second part of it in a separate PR.

dholbach added a commit to dholbach/flux that referenced this pull request Apr 9, 2019
	In fluxcd#1902 it was discovered that the part of the tutorial where
	PODINFO_UI_COLOR was changed didn't actually work. So we're
	using the same instructions as in the Helm tutorial now, modulo
	the namespace (default vs flux) change.

Signed-off-by: Daniel Holbach <daniel@weave.works>
@2opremio 2opremio merged commit d11f2d8 into fluxcd:master Apr 9, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: e2e-testing Your tests passed on CircleCI!
Details
@2opremio 2opremio deleted the 2opremio:helm-op-get-started branch Apr 9, 2019
dholbach added a commit to dholbach/flux that referenced this pull request Apr 9, 2019
	In fluxcd#1902 it was identified that the podinfo HTML
	did not change colour. If you use curl to get the
	JSON reply, the  color  value is updated though.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.