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

k8s: Use service account token by default and improve error logging #808

Merged
merged 4 commits into from
Jan 14, 2016

Conversation

2opremio
Copy link
Contributor

Fixes #717

@paulbellamy PTAL

@2opremio 2opremio force-pushed the 717-k8s-service-account-auth branch from 3a3c41c to fa43df2 Compare January 12, 2016 15:31
@tomwilkie
Copy link
Contributor

This will need updates to README to document how to use scope with k8s.

@paulbellamy
Copy link
Contributor

LGTM, and needs documentation.

@2opremio
Copy link
Contributor Author

This will need updates to README to document how to use scope with k8s.

I discussed this with @paulbellamy but we were not sure we should modify the documentation until we release it. The README exhorts the user to install the latest release, so documenting something we haven't released is going to be misleading.

@tomwilkie
Copy link
Contributor

Thats fine, but lets at least have the update ready before merging this. We'll have to do the whole next-release-readme branch thing.

@2opremio
Copy link
Contributor Author

Sure, I will create a separate branch with the updates. Let me do it before we merge this so that it can be reviewed together with this PR.

@2opremio 2opremio force-pushed the 717-k8s-service-account-auth branch 4 times, most recently from c3635e5 to 68419fd Compare January 13, 2016 18:00
@2opremio
Copy link
Contributor Author

@tomwilkie @paulbellamy I've included the documentation here after all since the users should simply use the new resource definitions at https://github.com/TheNewNormal/kube-charts/tree/master/weavescope/manifests

That should basically be the source of truth of how to use the flags which I will update once we release 0.12 with this PR.

@2opremio
Copy link
Contributor Author

CC @rimusz please have a look at the documentation update.

I really like Helm but due to the extra manual steps involved I am not completely sure it contributes to make the installation easier. The steps to install Helm and to add the extra repo probably don't pay off, since the user needs to invoke kubectl manually anyways (due to the optional app component and the deployment ordering restriction, which I believe cannot be enforced though helm install)

Please let me know if I missed something and helm install could be used in our usecase. Thanks.

@2opremio 2opremio force-pushed the 717-k8s-service-account-auth branch from 68419fd to 312b9ac Compare January 13, 2016 18:34
@rimusz
Copy link

rimusz commented Jan 13, 2016

@2opremio I said in the email I will make a PR to upstream Helm repo, so these steps will be redundant:

3. Add the kube-charts helm repo:

 helm up
 helm repo add kube-charts https://github.com/TheNewNormal/kube-charts
 helm up

Also using new helm v0.3 (template and generator options) should be possible to make scope install much easier with different requirements

@2opremio
Copy link
Contributor Author

@rimusz Good! Is there a possibility to define a deployment order and optional resources for helm install?

Without this we need to resort to use kubectl manually and helm ends up being used merely as a glorified curl.

@paulbellamy
Copy link
Contributor

The manifests look fantastic but, due to all the manual fiddling still needed, I'm not sure helm adds much yet. Let's see what the instructions look like without it, @2opremio?

@2opremio
Copy link
Contributor Author

I agree, we can include Helm again once:

  • The chart is merged upstream (no need for helm add repo)
  • Helm v0.3 is released (which may allow automating the customization of the probe arguments)
  • helm install supports setting a deployment order and optional resources.

I will modify the instructions to use curl for now. @rimusz thanks so much again for the resource definitions.

@2opremio 2opremio force-pushed the 717-k8s-service-account-auth branch from 499a5e1 to 65762ff Compare January 14, 2016 11:56
@2opremio
Copy link
Contributor Author

@rimusz Please let us know if you are OK with us using the resource definitions without Helm until the problems I outlined above are resolved.

Otherwise we are happy leaving the documentation as it was until they are resolved and we include Helm in the instructions.

@2opremio 2opremio force-pushed the 717-k8s-service-account-auth branch from 65762ff to 5633188 Compare January 14, 2016 12:35
@rimusz
Copy link

rimusz commented Jan 14, 2016

@2opremio sure, that's fine

@2opremio 2opremio force-pushed the 717-k8s-service-account-auth branch from 5633188 to e0dfeb1 Compare January 14, 2016 12:57
2opremio pushed a commit that referenced this pull request Jan 14, 2016
k8s: Use service account token by default and improve error logging
@2opremio 2opremio merged commit ada38a3 into master Jan 14, 2016
@2opremio 2opremio deleted the 717-k8s-service-account-auth branch January 14, 2016 13:01
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.

4 participants