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

Add 'config print'. #727

Merged
merged 4 commits into from
Jun 16, 2023
Merged

Add 'config print'. #727

merged 4 commits into from
Jun 16, 2023

Conversation

floitsch
Copy link
Member

@floitsch floitsch requested a review from kasperl June 15, 2023 20:41
Copy link
Contributor

@kasperl kasperl left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@@ -23,12 +23,15 @@ import fs
import .utils

APP_NAME ::= "artemis"

// When adding a new config don't forget to update the 'config show' command.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// When adding a new config don't forget to update the 'config show' command.
// When adding a new config don't forget to update the 'config print' command.

Copy link
Member Author

Choose a reason for hiding this comment

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

going with 'show'.

CONFIG_DEVICE_DEFAULT_KEY ::= "device.default"
CONFIG_BROKER_DEFAULT_KEY ::= "server.broker.default"
CONFIG_ARTEMIS_DEFAULT_KEY ::= "server.artemis.default"
CONFIG_SERVERS_KEY ::= "servers"
CONFIG_SERVER_AUTHS_KEY ::= "auths"
CONFIG_ORGANIZATION_DEFAULT_KEY ::= "organization.default"
// When adding a new config don't forget to update the 'config show' command.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// When adding a new config don't forget to update the 'config show' command.
// When adding a new config don't forget to update the 'config print' command.

Copy link
Member Author

Choose a reason for hiding this comment

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

going with 'show'.

--run=:: show_config config ui

config_cmd.add show_cmd
print_cmd := cli.Command "print"
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like we're just getting the config here. Printing isn't really that interesting. Also, we still use 'device show'.

For configuration options, we often have 'config xxx' to show and 'config xxx 1234' to set, no?

Also, see #472.

Copy link
Member Author

Choose a reason for hiding this comment

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

Going with show.

@floitsch floitsch enabled auto-merge (squash) June 16, 2023 14:06
@floitsch floitsch merged commit a5250b9 into main Jun 16, 2023
@floitsch floitsch deleted the floitsch/print_config branch June 16, 2023 14:44
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.

Exception on Artemis config show
2 participants