Skip to content

[bugfix, cleanup] Finish moving plugin services to process compose #1150

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

Merged
merged 2 commits into from
Jun 15, 2023

Conversation

ipince
Copy link
Contributor

@ipince ipince commented Jun 13, 2023

Summary

Removes services stanza from the plugin's json definition, in favor of using process-compose.yaml.

  • Fixes bug where, in doing devbox services ls, we would only return the first service defined in a plugin, and would call it the name of the plugin (instead of the name of the service). This had some downstream effects as well.
  • Removes creation of wrappers of the form <svc>-start|stop-service. They were using the json-based definitions of the services, but devbox service start <svc> was using the process-compose definition, so they could diverge. Some wrappers were not being generated either, due to the bug above.
  • Deletes obsolete code.
  • Updates plugins that define services and bump their versions. Since plugins are packaged in the CLI, and the process-compose yamls were already being written before this change, I think this change is backwards-compatible when it comes to plugins.

Reviewers, please advise on whether this change would affect service port-forwarding.

How was it tested?

Ran devbox services ls|start|stop|up with different plugins installed and verified they worked as expected.

Backwards-compatible?

NO, because we're removing the service wrappers (e.g. redis-start-service). I don't think they're being used, but @mikeland86 please confirm? If needed, I can replace them with devbox services start redis or something like that.

@ipince ipince requested review from mikeland73 and Lagoja June 13, 2023 21:34
Copy link
Contributor

@Lagoja Lagoja left a comment

Choose a reason for hiding this comment

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

LGTM, but i'd check with Mike on the bits that were removed

Copy link
Contributor

@mikeland73 mikeland73 left a comment

Choose a reason for hiding this comment

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

This is great! See my comment about printProxyURL which was accidentally removed a while back. I think we should add it in even if just generic.

We can figure out a different way of finding the port.

stopService
)

func printProxyURL(w io.Writer, services Services) error { // TODO: remove it?
Copy link
Contributor

Choose a reason for hiding this comment

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

The callsite to this was accidentally removed in https://github.com/jetpack-io/devbox/pull/836/files (I looked it up because I was surprised this was unused)

The purpose of this endpoint is to print the URL the user can use to proxy to their devbox.sh service. I think it's an important part of making devbox cloud usable.

That said, currently only a single plugin declares a port, so we may want to just show the generic message and figure out how to show the port later.

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'll address this in a separate PR.

@ipince ipince force-pushed the rodrigo/plugin-svcs branch from 643f7af to 41e2bf3 Compare June 15, 2023 16:11
@ipince ipince merged commit 9a400eb into main Jun 15, 2023
@ipince ipince deleted the rodrigo/plugin-svcs branch June 15, 2023 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants