Skip to content

Use ksonnet CLI instead of ksonnet libs (#590)#626

Merged
merenbach merged 12 commits intoargoproj:masterfrom
merenbach:590-use-ksonnet-cli
Sep 27, 2018
Merged

Use ksonnet CLI instead of ksonnet libs (#590)#626
merenbach merged 12 commits intoargoproj:masterfrom
merenbach:590-use-ksonnet-cli

Conversation

@merenbach
Copy link
Contributor

@merenbach merenbach commented Sep 20, 2018

Closes #590.

Major points:

  • Completely removes the Ksonnet client libs as a dependency.
  • Removes the ksonnetApp struct and KsonnetApp interface types and replaces with utility functions that accept a root dir path.
  • Upgrades to Ksonnet 0.13.0, which purports to fix v0.12 Regression: param values with double quotes are unexpectedly escaped ksonnet/ksonnet#861 (which prevented us from upgrading to 0.12.0). @jessesuen I have tested the new version and it appears to satisfy the repro tests you put into the bug you submitted, so I am so far satisfied that the issue with 0.12.0 is resolved.

Other points:

  • Replaces use of path lib with filepath since we're working with file paths, not just serialized object paths.

@merenbach merenbach added the help-wanted Extra attention is needed label Sep 20, 2018
@merenbach merenbach changed the title [WIP] Use ksonnet CLI and remove all mention of ksonnet libs (#590) [WIP] Use ksonnet CLI instead of ksonnet libs (#590) Sep 20, 2018
Copy link
Member

Choose a reason for hiding this comment

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

Let's not mix yaml packages, everything else is using ghodss/yaml.

Copy link
Member

Choose a reason for hiding this comment

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

I still prefer keeping around the ksonnet interface, especially for mocking, and consistency with the helm and kustomize implementations. It can have a reduced method list, but theres value in having it defined as an interface.

Gopkg.toml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Please don't add dependencies here -- it's stated clearly in the comment.

@jessesuen
Copy link
Member

@merenbach i blew away my vendor directory, ran dep ensure and was able to build everything. I'm not sure what the problem could be.

@merenbach
Copy link
Contributor Author

@jessesuen that's really interesting! Seems that @dthomson25 and I have some difference between our local setups and yours. I'm using the following Go version:

$ go version
go version go1.11 darwin/amd64

This is installed with Homebrew (brew install go), in case that makes any difference.

Copy link
Member

Choose a reason for hiding this comment

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

It seems all of the ksonnet methods have been switching to use a new package level ksCmd which accepts the ksonnet root as the first argument. The point of having a method-level ksCmd was so that the callers didn't have to keep passing the root dir. I don't think the package level ksCmd is necessary if the only reason it was made package-level was to be able to run ksonnet version without an app, in which case the KsonnetVersion() function could have been implemented using a throwaway app. e.g.:

ksApp := ksonnetApp{}	
ksApp.ksCmd("version")

@merenbach
Copy link
Contributor Author

Testing update: am able to do a basic sync on the Ksonnet guestbook. I am also able set a JSON parameter override, e.g.:

  source:
    componentParameterOverrides:
    - component: ks-guestbook
      name: helloworld
      value: '{"A":"B","C":4}'
    environment: default
    path: ksonnet-guestbook
    repoURL: https://github.com/argoproj/argocd-example-apps.git
    targetRevision: HEAD

Also able to override images, e.g.,:

$ ./dist/argocd app create --name ks-guestbook --repo https://github.com/argoproj/argocd-example-apps.git --path ksonnet-guestbook --env default -p guestbook-ui=image=gcr.io/heptio-images/ks-guestbook-demo:0.7

Output:

  operationState:
    finishedAt: 2018-09-26T19:37:47Z
    message: successfully synced
    operation:
      sync:
        parameterOverrides:
        - component: guestbook-ui
          name: image
          value: gcr.io/heptio-images/ks-guestbook-demo:0.7
        syncStrategy:
          hook: {}
    phase: Succeeded
    startedAt: 2018-09-26T19:37:44Z
    syncResult:
      resources:
      - kind: Service
        message: service/ks-guestbook-ui created
        name: ks-guestbook-ui
        namespace: default
        status: Synced
      - kind: Deployment
        message: deployment.apps/ks-guestbook-ui created
        name: ks-guestbook-ui
        namespace: default
        status: Synced
      revision: d9c52b4c89474bf6f7c33694b7650e54b60944b4

@merenbach merenbach requested a review from alexmt September 26, 2018 19:38
@merenbach merenbach changed the title [WIP] Use ksonnet CLI instead of ksonnet libs (#590) Use ksonnet CLI instead of ksonnet libs (#590) Sep 26, 2018
@merenbach merenbach dismissed jessesuen’s stale review September 26, 2018 19:44

Fixed issues presented so far

Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

One minor comment. LGTM after fixing it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Default revision should be set before executing repoClient.GetFile (line 328)

Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

LGTM

@merenbach merenbach merged commit 1eaa813 into argoproj:master Sep 27, 2018
@merenbach merenbach deleted the 590-use-ksonnet-cli branch September 27, 2018 18:52
leoluz pushed a commit to leoluz/argo-cd that referenced this pull request Mar 13, 2025
* chore: avoid unnecessary json marshal

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* more tests

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* refactor test to satisfy sonarcloud

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

help-wanted Extra attention is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use ksonnet CLI instead of ksonnet lib v0.12 Regression: param values with double quotes are unexpectedly escaped

3 participants