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

Adding helm service endpoints to console #3826

Merged
merged 2 commits into from Jan 24, 2020

Conversation

akashshinde
Copy link
Contributor

@akashshinde akashshinde commented Dec 23, 2019

NOTE: Need to get #3992 and #4007 merged before this PR.

This PR adds following helm service routes CC: @sbose78 @pedjak

  1. /api/helm/releases - List all installed charts
  2. /api/helm/template - Simulates helm template and renders manifest files.
  3. /api/helm/release - Installs helm chart

To Test

  1. Get Releases GET /api/helm/releases
    Test : curl http://localhost:9000/api/helm/releases

  2. Install Release POST /api/helm/release
    Test: curl -X POST http://localhost:9000/api/helm/release
    Body

{
	"name": "test-helm",
	"namespace": "akash-helm-server",
	"chart_url": "https://github.com/akashshinde/console/raw/helm_endpoints/pkg/helm/testdata/influxdb-3.0.2.tgz"
}

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. component/backend Related to backend labels Dec 23, 2019
@benjaminapetersen
Copy link
Contributor

/hold

e2es are 100% failing

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 23, 2019
@sbose78
Copy link

sbose78 commented Dec 30, 2019

Cloning "https://github.com/akashshinde/console.git" ...
	Commit:	eff0686d14819b72634bdd4b58a2dedbb9aa4906 (go fmt)
	Author:	Akash Shinde <akashshinde159@gmail.com>
	Date:	Mon Dec 23 14:47:05 2019 +0530
Caching blobs under "/var/cache/blobs".

Pulling image openshift/origin-base ...
Getting image source signatures
Copying blob sha256:b2d2704dda6c98b6a775e8e5566af7a92f700b542905bd94310dc18b9bc2d0b0
Copying blob sha256:6e55351c18ffebc0918b4c21c1257d28d7e311bd37009fb722cb59761b7449ed
Copying blob ..
..
sha256:f17c3afdd8863b7b92d12e0a444b92dc76e56667d264ccc60882eed91b5627e6
Writing manifest to image destination
Storing signatures
STEP 1: FROM quay.io/coreos/tectonic-console-builder:v18 AS build
STEP 2: RUN mkdir -p /go/src/github.com/openshift/console/
4057c29e8afa2f3e11d6b4d0beec5d327f3a18e2e8dc68214c13e0955e862f53
STEP 3: ADD . /go/src/github.com/openshift/console/
54a988edc43ef2d9d9f95f7a2543cb1ac0c4c19d28c39c094d8ba9d7b415b373
STEP 4: WORKDIR /go/src/github.com/openshift/console/
41527da406326440983ea12947629fab8788db4038468bd4b194bd5d90a98fb4
STEP 5: RUN ./build.sh
# github.com/openshift/console/vendor/helm.sh/helm/v3/pkg/chart/loader
vendor/helm.sh/helm/v3/pkg/chart/loader/archive.go:145:7: undefined: strings.ReplaceAll
# github.com/openshift/console/vendor/helm.sh/helm/v3/pkg/kube
vendor/helm.sh/helm/v3/pkg/kube/client.go:520:21: undefined: strings.ReplaceAll
subprocess exited with status 2
subprocess exited with status 2
error: build error: error building at STEP "RUN ./build.sh": exit status 2

Tried building an image using https://github.com/openshift/console/blob/master/Dockerfile , the build failed with the above error.

Note: ReplaceAll(..) was added in 1.12. quay.io/coreos/tectonic-console-builder:v18 probably has a pre-1.12 golang version? https://github.com/openshift/console/blob/master/Dockerfile.builder#L5

Copy link

@sbose78 sbose78 left a comment

Choose a reason for hiding this comment

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

How about setting up the namespace context upfront?

pkg/helm_actions/install_chart.go Outdated Show resolved Hide resolved
pkg/helm_actions/list_releases.go Outdated Show resolved Hide resolved
pkg/helm_actions/upgrade_release.go Outdated Show resolved Hide resolved
pkg/helm_agent/agent.go Outdated Show resolved Hide resolved
pkg/helm/agent.go Outdated Show resolved Hide resolved
pkg/helm/install_chart_test.go Outdated Show resolved Hide resolved
pkg/helm/install_chart_test.go Outdated Show resolved Hide resolved
pkg/helm/list_releases.go Outdated Show resolved Hide resolved
pkg/helm/list_releases_test.go Outdated Show resolved Hide resolved
pkg/server/server.go Outdated Show resolved Hide resolved
pkg/helm/install_chart.go Outdated Show resolved Hide resolved
pkg/helm/template.go Outdated Show resolved Hide resolved
pkg/server/server.go Outdated Show resolved Hide resolved
pkg/server/server.go Outdated Show resolved Hide resolved
@spadgett
Copy link
Member

/assign @benjaminapetersen

@@ -417,3 +422,37 @@ func (s *Server) handleOpenShiftTokenDeletion(user *auth.User, w http.ResponseWr
io.Copy(w, resp.Body)
resp.Body.Close()
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to move your handlers into your helm package? A handlers.go file would be fine, and just load them up via imports.

server.go doesn't need to know any details about helm, it can include the 3 lines setting up the handle("api/console/helm/....", authHandlerWithUser( helmcatalog.InstallHandler ). etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would still like to see this change.

Copy link
Contributor

@benjaminapetersen benjaminapetersen left a comment

Choose a reason for hiding this comment

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

Couple items organizationally.

@benjaminapetersen
Copy link
Contributor

Suggested a couple small changes:

Unless it makes sense to have more than 3 commits, this is probably the best pattern.

@sbose78
Copy link

sbose78 commented Jan 16, 2020

/retest

@akashshinde
Copy link
Contributor Author

akashshinde commented Jan 16, 2020

Suggested a couple small changes:

Done renamed to helmcatalog. @sbose78 should we keep it helm only ?

As of now helm handler uses TLSConfig from Server obj, also handler uses sendResponse/apiError which are private in server package. This would need duplication of code.
@benjaminapetersen what do you think ?

  • Squash you commits to the following:

    • dependencies + go.mod/go.sum
    • feature code
    • test code

Added commits the way you suggested, I've been working on adding tests.

@pedjak
Copy link
Contributor

pedjak commented Jan 16, 2020

@sbose78 @akashshinde I would name the package simply helm - IMHO, not seeing any additional benefits appending catalog to the name. We are crafting here a service that exposes helm functionalities, not only catalog functionalities. We could make it more fine-grained later if needed. #3928 introduces helm package as well.

@sbose78
Copy link

sbose78 commented Jan 16, 2020

@pedjak Agreed, go for it.

@akashshinde
Copy link
Contributor Author

/retest

Copy link
Contributor

@benjaminapetersen benjaminapetersen left a comment

Choose a reason for hiding this comment

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

See comments, thanks!

pkg/server/server.go Outdated Show resolved Hide resolved
@akashshinde
Copy link
Contributor Author

/test images

@akashshinde
Copy link
Contributor Author

/test e2e-gcp-console

@pedjak
Copy link
Contributor

pedjak commented Jan 23, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2020
@akashshinde
Copy link
Contributor Author

/test e2e-gcp-console

@akashshinde
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 23, 2020
@benjaminapetersen
Copy link
Contributor

Ok, give this one a squash down to 2 commits:

  • add deps for help service endpoints
  • add helm service endpoints

And we can take a final look, thanks!

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2020
@sbose78
Copy link

sbose78 commented Jan 23, 2020

/test analyze

plog = capnslog.NewPackageLogger("github.com/openshift/console", "serverutils")
)

// Copied from Server package to maintain error response consistency
Copy link
Contributor

Choose a reason for hiding this comment

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

A follow, we can eliminate this comment since its no longer copied. This is the only occurance.

@benjaminapetersen
Copy link
Contributor

/retest

NoSuchElementError: Index out of bound. Trying to access element at index: 0, but there are only 0 elements that match locator By(css selector, [placeholder="value"])
    at selenium_webdriver_1.promise.all.then (/go/src/github.com/openshift/console/frontend/node_modules/protractor/built/element.js:274:27)
    at ManagedPromise.invokeCallback_ (/go/src/github.com/openshift/console/frontend/node_modules/selenium-webdriver/lib/promise.js:1376:14)
    at TaskQueue.execute_ (/go/src/github.com/openshift/console/frontend/node_modules/selenium-webdriver/lib/promise.js:3084:14)
    at TaskQueue.executeNext_ (/go/src/github.com/openshift/console/frontend/node_modules/selenium-webdriver/lib/promise.js:3067:27)
    at asyncRun (/go/src/github.com/openshift/console/frontend/node_modules/selenium-webdriver/lib/promise.js:2927:27)
    at /go/src/github.com/openshift/console/frontend/node_modules/selenium-webdriver/lib/promise.js:668:7
    at process._tickCallback (internal/process/next_tick.js:68:7)Error
    at ElementArrayFinder.applyAction_ (/go/src/github.com/openshift/console/frontend/node_modules/protractor/built/element.js:459:27)
    at ElementArrayFinder.(anonymous function).args [as getAttribute] (/go/src/github.com/openshift/console/frontend/node_modules/protractor/built/element.js:91:29)
    at ElementFinder.(anonymous function).args [as getAttribute] (/go/src/github.com/openshift/console/frontend/node_modules/protractor/built/element.js:831:22)
    at /go/src/github.com/openshift/console/frontend/integration-tests/tests/modal-annotations.scenario.ts:70:75
    at /go/src/github.com/openshift/console/frontend/integration-tests/protractor.conf.ts:267:18
    at step (/go/src/github.com/openshift/console/frontend/integration-tests/protractor.conf.ts:33:23)
    at Object.next (/go/src/github.com/openshift/console/frontend/integration-tests/protractor.conf.ts:14:53)
    at /go/src/github.com/openshift/console/frontend/integration-tests/protractor.conf.ts:8:71
    at new ManagedPromise (/go/src/github.com/openshift/console/frontend/node_modules/selenium-webdriver/lib/promise.js:1077:7)
    at __awaiter (/go/src/github.com/openshift/console/frontend/integration-tests/protractor.conf.ts:4:12)

Your code doesn't touch the frontend, flake.

@benjaminapetersen
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akashshinde, benjaminapetersen, pedjak

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2020
@akashshinde
Copy link
Contributor Author

/test e2e-gcp-console

@openshift-merge-robot openshift-merge-robot merged commit b62c1df into openshift:master Jan 24, 2020
@spadgett spadgett added this to the v4.4 milestone Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/backend Related to backend lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants