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

Remove assetsvc #4880

Merged
merged 10 commits into from
Jun 9, 2022
Merged

Remove assetsvc #4880

merged 10 commits into from
Jun 9, 2022

Conversation

antgamdia
Copy link
Contributor

@antgamdia antgamdia commented Jun 8, 2022

Description of the change

This PR finally removes the assetsvc code (note that we deprecated it in versionv2.4.0). This deletion paves the way to tidy up the pkg folder: there is some logic used by both the assetsvc and the helm plugin; now this logic can be safely moved into the kubeapps-apis directory. I'll perform those changes in upcoming PRs.

Benefits

Less unused code !

Possible drawbacks

N/A (hopefully :P)

Applicable issues

Additional information

will investigate the CI issues tomorrow

antgamdia added 4 commits June 8, 2022 19:25
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@netlify
Copy link

netlify bot commented Jun 8, 2022

Deploy Preview for kubeapps-dev canceled.

Name Link
🔨 Latest commit 6916f19
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/62a2126d8231f70008d7d94a

antgamdia added 3 commits June 8, 2022 23:06
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

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

!!!! Woohoo. Thanks for taking care of moving the pg utils work for now.

@castelblanque
Copy link
Collaborator

Thanks! Now Kubeapps feels ligther 😃

@antgamdia
Copy link
Contributor Author

antgamdia commented Jun 9, 2022

The current CI issue seems to be a problem when upgrading from a chart that is passing an old flag. Let's see if we can ignore this failure (no, it is not supported by Cobra) or maybe just add the param back until we release a new version.

circleci@ip-172-28-92-82:~/project$ k logs -n kubeapps kubeapps-ci-internal-kubeops-987c457df-lk82s
Error: unknown flag: --assetsvc-url
Usage:
  kubeops [flags]

Flags:
      --burst int                         internal burst capacity (default 15)
      --clusters-config-path string       Configuration for clusters
      --global-repos-namespace string     Namespace of global repositories (default "kubeapps")
      --helm-driver string                which Helm driver type to use
  -h, --help                              help for kubeops
      --list-max int                      maximum number of releases to fetch (default 256)
      --namespace-header-name string      name of the header field, e.g. namespace-header-name=X-Consumer-Groups     
      --namespace-header-pattern string   regular expression that matches only single group, e.g. namespace-header-pattern=^namespace:([\w]+):\w+$, to match namespace:ns:read
      --pinniped-proxy-url string         internal url to be used for requests to clusters configured for credential proxying via pinniped (default "http://kubeapps-internal-pinniped-proxy.kubeapps:3333")
      --qps float32                       internal QPS rate (default 10)
      --timeout int                       Timeout to perform release operations (install, upgrade, rollback, delete) (default 300)
      --user-agent-comment string         UserAgent comment used during outbound requests
  -v, --version                           version for kubeops

Error: unknown flag: --assetsvc-url

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Comment on lines +55 to +60
// TODO(agamez): remove this after the v2.4.6 release
var assetsvcurlDeprecated string

func setFlags(c *cobra.Command) {
c.Flags().StringVar(&serveOpts.AssetsvcURL, "assetsvc-url", "https://kubeapps-internal-assetsvc:8080", "URL to the internal assetsvc")
// TODO(agamez): remove 'assetsvc-url' after the v2.4.6 release
c.Flags().StringVar(&assetsvcurlDeprecated, "assetsvc-url", "", "(DEPRECATED)")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an ugly workaround to get the upgrade test passing... otherwise (see my previous comment), they're gonna fail in both our CI and Bitnami's.
Once we release v2.4.6, we can safely delete it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is fine as long as we don't forget to remove it in a later stage.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm adding a card to our project dashboard... I can't rely on my memory 👴

* Fix "redundant type from array" warning

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Clean pkg/auth dir

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Clean more pkg/*

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Move pkg/response to kubeops

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Move pkg/http-handler to kubeops

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Move pkg/auth to kubeops

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants