Get rid of .git-credentials and switch to GCS #300
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merge Upstream
…s synced from google cloud storage
Upstream Merge
# Conflicts: # pkg/kudoctl/util/check/check.go # pkg/kudoctl/util/github/client_test.go
Reduce the scope of the cli improvement branch
into new helper method
Remove remaining git-credentials stuff
Read bundles from memory, no local repo. Small fixes
fabianbaier
reviewed
Jun 5, 2019
runyontr
requested changes
Jun 6, 2019
| if err != nil { | ||
| return nil, errors.Wrap(err, "parsing config url") | ||
| } | ||
| parsedURL.Path = parsedURL.Path + "/" + bundleName + ".tgz" |
There was a problem hiding this comment.
What's the point of the index file if the bundle can be determined by a name? I had expected an index file to be required here to get the location of the bundle.
There was a problem hiding this comment.
Exactly. We need to update the KEP accordingly but this is also been captured/discussed in Slack, e.g. here https://kubernetes.slack.com/archives/CG3HTFCMV/p1559574030076700
To put it into other words, having the index file urls field used to point to the download location of the tgz file. It also gives you the option for multiple mirrors and some redundancy.
What we would need here is traversing through the urls field, e.g. trying the first url, if it fails then trying the next and so on until the download succeeds. Wdyt?
gerred
requested changes
Jun 6, 2019
gerred
requested changes
Jun 6, 2019
| @@ -26,6 +27,16 @@ func TestInstallCmd(t *testing.T) { | |||
| cmdWrongDirKubeConfigFlag := &cobra.Command{} | |||
| cmdWrongDirKubeConfigFlag.Flags().StringVar(&vars.KubeConfigPath, "kubeconfig", "", "Usage") | |||
| vars.KubeConfigPath = "/tmp" | |||
| index := []byte(`apiVersion: v1 | |||
There was a problem hiding this comment.
It would be really good practice here to use the functions directly, and have them take an io.ReadWriteCloser or some other interface. That way, we can use a mock/virtual filesystem instead of doing this for real. Our cmd function would then be responsible for converting a filename to an io.ReadWriteCloser.
This would also get side effects out of unit tests (though we could preserve it for e2e, and turn this into an e2e test).
I wouldn't change for now, but happy to pair through it next week!
gerred
approved these changes
Jun 6, 2019
zen-dog
approved these changes
Jun 6, 2019
pkg/kudoctl/cmd/install.go
Outdated
| installCmd.Flags().StringVar(&vars.Namespace, "namespace", "default", "The namespace where the operator watches for changes. (default to") | ||
| installCmd.Flags().StringArrayVarP(&vars.Parameter, "parameter", "p", nil, "The parameter name and value separated by '='") | ||
| installCmd.Flags().StringVar(&vars.Namespace, "namespace", "default", "The namespace used for the framework installation. (default \"default\"") | ||
| installCmd.Flags().StringArrayVarP(&vars.Parameter, "parameter", "p", nil, "The parameter name.") |
There was a problem hiding this comment.
Was this a merge conflict? Because line on the left is correct
pkg/kudoctl/cmd/install/install.go
Outdated
| @@ -275,20 +259,19 @@ func installSingleInstanceToCluster(name, previous, path string, gc *github.Clie | |||
|
|
|||
| // This checks if flag --instance was set with a name and it is the not a dependency Instance | |||
| if vars.Instance != "" && previous == "" { | |||
| instanceYaml.ObjectMeta.SetName(vars.Instance) | |||
| i.ObjectMeta.SetName(vars.Instance) | |||
There was a problem hiding this comment.
i in loops or as a short form for index is ok ^^ But I'd prefer instance here.
|
|
||
| bundle, err := untar(resp) | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
Should we add some bundle sanity checking here? E.g. that all files are set?
|
Closes #273 |
runyontr
approved these changes
Jun 6, 2019
14 tasks
fabianbaier
requested changes
Jun 6, 2019
There was a problem hiding this comment.
Not sure how and why but now I can't install a Framework anymore.
$ go run cmd/kubectl-kudo/main.go install kafka --all-dependenciesNo Instance tied to this "zookeeper" version has been found. Do you want to create one? (Yes/no)
No Instance tied to this "kafka" version has been found. Do you want to create one? (Yes/no)
Because of
2019/06/06 13:38:37 PlanExecutionController: Error expanding template: error parsing template: template: tpl:5: function "NAMESPACE" not defined
2019/06/06 13:38:37 PlanExecutionController: Error expanding template: error parsing template: template: tpl:4: function "NAME" not defined
2019/06/06 13:38:37 PlanExecutionController: Error expanding template: error parsing template: template: tpl:4: function "NAME" not defined
and
2019/06/06 13:40:59 PlanExecutionController: PlanExecution "kafka-deploy-228717000" has already run to completion, not processing.
I need to do more digging what changed, probably because we added/merged some other PRs that need to be reflected here or in the overall frameworks itself.
Controller output verbose
$ make run
go run -ldflags "-X github.com/kudobuilder/kudo/pkg/version.gitVersion=v0.1.0 -X github.com/kudobuilder/kudo/pkg/version.gitCommit=8a7a1b44e9a69ba59c2773afb051168327ce3cd1 -X github.com/kudobuilder/kudo/pkg/version.buildDate=2019-06-06T18:47:35Z" ./cmd/manager/main.go
go: downloading github.com/masterminds/sprig v2.18.0+incompatible
go: extracting github.com/masterminds/sprig v2.18.0+incompatible
go: downloading github.com/Masterminds/goutils v1.1.0
go: downloading github.com/huandu/xstrings v1.2.0
go: extracting github.com/Masterminds/goutils v1.1.0
go: extracting github.com/huandu/xstrings v1.2.0
{"level":"info","ts":1559853071.5527182,"logger":"entrypoint","msg":"KUDO Version: version.Info{GitVersion:\"v0.1.0\", GitCommit:\"8a7a1b44e9a69ba59c2773afb051168327ce3cd1\", BuildDate:\"2019-06-06T18:47:35Z\", GoVersion:\"go1.12.4\", Compiler:\"gc\", Platform:\"darwin/amd64\"}"}
{"level":"info","ts":1559853071.55285,"logger":"entrypoint","msg":"setting up client for manager"}
{"level":"info","ts":1559853071.554379,"logger":"entrypoint","msg":"setting up manager"}
{"level":"info","ts":1559853071.5911648,"logger":"entrypoint","msg":"Registering Components."}
{"level":"info","ts":1559853071.591191,"logger":"entrypoint","msg":"setting up scheme"}
{"level":"info","ts":1559853071.591325,"logger":"entrypoint","msg":"Setting up controller"}
2019/06/06 13:31:11 FrameworkController: Registering framework controller.
{"level":"info","ts":1559853071.591452,"logger":"kubebuilder.controller","msg":"Starting EventSource","controller":"framework-controller","source":"kind source: /, Kind="}
2019/06/06 13:31:11 FrameworkVersionController: Registering frameworkversion controller.
{"level":"info","ts":1559853071.5916858,"logger":"kubebuilder.controller","msg":"Starting EventSource","controller":"frameworkversion-controller","source":"kind source: /, Kind="}
2019/06/06 13:31:11 InstanceController: Registering instance controller.
{"level":"info","ts":1559853071.591894,"logger":"kubebuilder.controller","msg":"Starting EventSource","controller":"instance-controller","source":"kind source: /, Kind="}
2019/06/06 13:31:11 PlanExecutionController: Registering planexecution controller.
{"level":"info","ts":1559853071.592053,"logger":"kubebuilder.controller","msg":"Starting EventSource","controller":"planexecution-controller","source":"kind source: /, Kind="}
{"level":"info","ts":1559853071.592223,"logger":"kubebuilder.controller","msg":"Starting EventSource","controller":"planexecution-controller","source":"kind source: /, Kind="}
{"level":"info","ts":1559853071.592334,"logger":"kubebuilder.controller","msg":"Starting EventSource","controller":"planexecution-controller","source":"kind source: /, Kind="}
{"level":"info","ts":1559853071.592455,"logger":"kubebuilder.controller","msg":"Starting EventSource","controller":"planexecution-controller","source":"kind source: /, Kind="}
{"level":"info","ts":1559853071.593584,"logger":"kubebuilder.controller","msg":"Starting EventSource","controller":"planexecution-controller","source":"kind source: /, Kind="}
{"level":"info","ts":1559853071.593795,"logger":"entrypoint","msg":"setting up webhooks"}
{"level":"info","ts":1559853071.593888,"logger":"entrypoint","msg":"Starting the Cmd."}
2019/06/06 13:31:11 PlanExecutionController: Received create event for an instance named: coredns
{"level":"info","ts":1559853071.696341,"logger":"kubebuilder.controller","msg":"Starting Controller","controller":"framework-controller"}
{"level":"info","ts":1559853071.696335,"logger":"kubebuilder.controller","msg":"Starting Controller","controller":"frameworkversion-controller"}
{"level":"info","ts":1559853071.696335,"logger":"kubebuilder.controller","msg":"Starting Controller","controller":"instance-controller"}
{"level":"info","ts":1559853071.6963358,"logger":"kubebuilder.controller","msg":"Starting Controller","controller":"planexecution-controller"}
{"level":"info","ts":1559853071.797527,"logger":"kubebuilder.controller","msg":"Starting workers","controller":"instance-controller","worker count":1}
{"level":"info","ts":1559853071.797527,"logger":"kubebuilder.controller","msg":"Starting workers","controller":"framework-controller","worker count":1}
{"level":"info","ts":1559853071.7975428,"logger":"kubebuilder.controller","msg":"Starting workers","controller":"planexecution-controller","worker count":1}
{"level":"info","ts":1559853071.800935,"logger":"kubebuilder.controller","msg":"Starting workers","controller":"frameworkversion-controller","worker count":1}
2019/06/06 13:32:04 FrameworkController: Received Reconcile request for a framework named: kafka
2019/06/06 13:32:04 FrameworkVersionController: Received Reconcile request for a frameworkversion named: kafka-0.1.0
2019/06/06 13:32:04 FrameworkController: Received Reconcile request for a framework named: zookeeper
2019/06/06 13:32:04 FrameworkVersionController: Received Reconcile request for a frameworkversion named: zookeeper-0.1.0
2019/06/06 13:38:37 PlanExecutionController: Received create event for an instance named: zk
2019/06/06 13:38:37 InstanceController: Received create event for an instance named: zk
2019/06/06 13:38:37 InstanceController: Received Reconcile request for "zk"
2019/06/06 13:38:37 PlanExecutionController: Received update event for an instance named: zk
2019/06/06 13:38:37 InstanceController: Old and new spec matched...
2019/06/06 13:38:37 InstanceController: Going to call plan "deploy"
2019/06/06 13:38:37 InstanceController: Received Reconcile request for "zk"
2019/06/06 13:38:37 PlanExecutionController: Error expanding template: error parsing template: template: tpl:5: function "NAMESPACE" not defined
2019/06/06 13:38:37 PlanExecutionController: Error expanding template: error parsing template: template: tpl:4: function "NAME" not defined
2019/06/06 13:38:37 PlanExecutionController: Error expanding template: error parsing template: template: tpl:4: function "NAME" not defined
2019/06/06 13:38:37 PlanExecutionController: Phase "zookeeper" Step "everything" has 0 object(s)
2019/06/06 13:38:37 PlanExecutionController: Phase "zookeeper" has strategy parallel
2019/06/06 13:38:37 PlanExecutionController: Looked at step "everything"
2019/06/06 13:38:37 HealthUtil: Phase zookeeper is healthy
2019/06/06 13:38:37 PlanExecutionController: Phase "zookeeper" marked as healthy
2019/06/06 13:38:37 HealthUtil: Phase zookeeper is healthy
2019/06/06 13:38:37 PlanExecutionController: Received update event for an instance named: zk
2019/06/06 13:38:37 InstanceController: Old and new spec matched...
2019/06/06 13:38:37 InstanceController: Going to call plan "deploy"
2019/06/06 13:38:37 InstanceController: Received Reconcile request for "zk"
2019/06/06 13:38:37 PlanExecutionController: PlanExecution "zk-deploy-830919000" has already run to completion, not processing.
|
|
||
| // RepositoryConfiguration represents a collection of parameters for framework repository | ||
| type RepositoryConfiguration struct { | ||
| //LocalPath string `json:"localPath"` |
There was a problem hiding this comment.
remove this line entirely?
| case tar.TypeDir: | ||
| // we don't handle folders right now, the structure is flat | ||
|
|
||
| // if it's a file create it |
There was a problem hiding this comment.
this is actually not creating files anymore. // if its a file unmarshall it and add to result ?
|
|
||
| // sortPackages sorts the entries by version in descending order. | ||
| // | ||
| // In canonical form, the individual version records should be sorted so that |
There was a problem hiding this comment.
Can we change the comment here and make it more compact?
| return i, nil | ||
| } | ||
|
|
||
| // GetByName returns framework of given name. |
There was a problem hiding this comment.
s/framework/the framework
s/given/ a given
| return i.getFramework(name, constraint) | ||
| } | ||
|
|
||
| // GetByNameAndVersion returns the framework of given name and version. |
pkg/kudoctl/cmd/install/install.go
Outdated
| return errors.Wrapf(err, "installing %s-framework.yaml", name) | ||
| } | ||
| fmt.Printf("framework.%s/%s created\n", frameworkYaml.APIVersion, frameworkYaml.Name) | ||
| log.Infof("framework.%s/%s created\n", f.APIVersion, f.Name) |
There was a problem hiding this comment.
When installing a framework it would look now like this e.g.:
$ go run cmd/kubectl-kudo/main.go install kafka --all-dependenciesNo Instance tied to this "zookeeper" version has been found. Do you want to create one? (Yes/no)
No Instance tied to this "kafka" version has been found. Do you want to create one? (Yes/no)
instead of the kubectl used style with
framework.kudo.k8s.io "kafka" created
frameworkversion.kudo.k8s.io "kafka-0.1.0" created
instance.kudo.k8s.io "kafka" created
I just noticed and am indifferent here, probably the output should anyways be an IOStreams we add later on and this could pave the way. Wdyt?
There was a problem hiding this comment.
Also because when framework and frameworkversion is already installed we print out:
framework.kudo.k8s.io/kafka unchanged
frameworkversion.kudo.k8s.io/kafka unchanged
framework.kudo.k8s.io/zookeeper unchanged
frameworkversion.kudo.k8s.io/zookeeper unchanged
that should then maybe be not printed out as well.
Fixed and see here https://kubernetes.slack.com/archives/CG3HTFCMV/p1559854200091000 |
fabianbaier
previously requested changes
Jun 6, 2019
go.mod
Outdated
| github.com/google/gofuzz v0.0.0-20170612174753-24818f796faf // indirect | ||
| github.com/google/martian v2.1.0+incompatible |
There was a problem hiding this comment.
do we need to introduce this here?
There was a problem hiding this comment.
good catch. That was a PR feedback from Gerred, but I was supposed to use whatever controller uses and I screw it up :) Now that I see what controller uses (it's a controller-runtime kubernetes log libbrary) I would probably stick with printing out for now and introduce a logging library (like logrus maybe?) in a different PR :)
pkg/kudoctl/cmd/install/install.go
Outdated
| @@ -4,10 +4,13 @@ import ( | |||
| "fmt" | |||
| "strings" | |||
|
|
|||
| "github.com/google/martian/log" | |||
There was a problem hiding this comment.
Not sure if we should introduce this here and stay with printing it out for 0.2.0
alenkacz
changed the title
kensipe
requested changes
Jun 7, 2019
| --- | ||
| # Framework repository | ||
|
|
||
| KUDO CLI comes with built-in official repository of verified frameworks. Every time you use `kudo install ...` command, it always pulls package from this repository. |
|
|
||
| cred, err := github.GetGithubCredentials() | ||
| // Initializing empty repo with given variables | ||
| r, err := repo.NewFrameworkRepository(e) |
| if err != nil { | ||
| return errors.Wrap(err, "sorting most recent content dir") | ||
| } | ||
| func verifySingleFramework(name, previous string, r repo.FrameworkRepository, i *repo.IndexFile, kc *kudo.Client) error { |
There was a problem hiding this comment.
i seems like an index... not an indexFile. can we expand some of these vars to something that is easier to read please?
|
I guess we will have to update any concerns separately |
|
lgtm! 👍 :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/component kudoctl
What this PR does / why we need it:
We switched from github framework repository to GCS to get rid of gitcredentials.
For now
Which issue(s) this PR fixes:
Fixes #273
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Co-authored-by: Fabian Baier fabian@mesosphere.io
Co-authored-by: Aleksey Dukhovniy adukhovniy@mesosphere.io