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

feat(spaces): migrate drains:{set,get} to spaces topic #2433

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mble-sfdc
Copy link

This commit partially migrates the drains:set and drains:get commands for spaces under the spaces topic, so they can be accessed from spaces:drains:set and spaces:drains:get.

The rationale here is that these are spaces-specific commands, and there is significant semantic overlap between drains:add and drains:set. This makes it clearer that these commands are spaces-specific.

In order to not break comaptibility with existing workflows, the old drains:{set,get} commands will continue to work, but remain hidden.

@mble-sfdc mble-sfdc requested a review from a team as a code owner August 11, 2023 10:21
This commit partially migrates the `drains:set` and `drains:get`
commands for `spaces` under the `spaces` topic, so they can be accessed
from `spaces:drains:set` and `spaces:drains:get`.

The rationale here is that these are `spaces`-specific commands, and
there is significant semantic overlap between `drains:add` and
`drains:set`. This makes it clearer that these commands are
`spaces`-specific.

In order to not break comaptibility with existing workflows, the old
`drains:{set,get}` commands will continue to work, but remain hidden.
@mble-sfdc mble-sfdc force-pushed the mble-sfdc-space-drain-set branch from 3b959d2 to 57d306a Compare August 11, 2023 14:35
@mble-sfdc mble-sfdc temporarily deployed to AcceptanceTests August 11, 2023 14:35 — with GitHub Actions Inactive
@mble-sfdc mble-sfdc temporarily deployed to AcceptanceTests August 11, 2023 14:35 — with GitHub Actions Inactive
@mble-sfdc mble-sfdc temporarily deployed to AcceptanceTests August 11, 2023 14:35 — with GitHub Actions Inactive
@chrismarino
Copy link

chrismarino commented Aug 17, 2023

Drains can be set for Common Runtime apps, so we can't hide the old command. Very weird to have to use spaces:drains to set for a CR app. Even if we did this, our doc pages would have to change as well.

@chrismarino
Copy link

So, I could imagine there being two places to set log drains because one is specific to an app (drains:set) , while the other is set for the entire space (space:drain:set) as suggested here.

Not sure that is an improvement over the current drains:set --space

Lets hold off on this until we can get a more complete perspective on what this means for DX.

@mble-sfdc
Copy link
Author

mble-sfdc commented Aug 17, 2023

@chrismarino I think you might have fallen into the trap that this PR looks to mitigate.

This change (spaces:drains:set, spaces:drains:get) only acts on the space-specific semantic (drains:set, drains:get). For non-spaces apps, that uses drains:add. Here is the help output of the spaces-specific commands:

$ heroku help drains:get
display the log drain for a space

USAGE
  $ heroku drains:get -s <value> [--json]

FLAGS
  -s, --space=<value>  (required) space for which to get log drain
  --json               output in json format

DESCRIPTION
  display the log drain for a space
$ heroku help drains:set
replaces the log drain for a space

USAGE
  $ heroku drains:set URL -s <value>

FLAGS
  -s, --space=<value>  (required) space for which to set log drain

DESCRIPTION
  replaces the log drain for a space

Both require a --space flag.

Then the non-spaces-specific:

$ heroku help drains
display the log drains of an app

USAGE
  $ heroku drains -a <value> [--json] [-r <value>]

FLAGS
  -a, --app=<value>     (required) app to run command against
  -r, --remote=<value>  git remote of app to use
  --json                output in json format

DESCRIPTION
  display the log drains of an app


COMMANDS
  drains:add     adds a log drain to an app
  drains:remove  removes a log drain from an app
$ heroku help drains:add
adds a log drain to an app

USAGE
  $ heroku drains:add URL -a <value> [-r <value>]

FLAGS
  -a, --app=<value>     (required) app to run command against
  -r, --remote=<value>  git remote of app to use

DESCRIPTION
  adds a log drain to an app

The purpose of this PR is to make it very clear that drains:set and drains:get only apply within a spaces context, hence moving them under the spaces topic.

FWIW, drains:get and drains:set are already hidden commands - this PR doesn't change that, only adding the alias in the spaces topic (e.g. spaces:drains:set).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants