Skip to content
This repository has been archived by the owner on Feb 18, 2021. It is now read-only.

CLI: fix defaults; make consistent with admin cli #246

Merged
merged 5 commits into from
Jul 7, 2017
Merged

Conversation

kirg
Copy link
Contributor

@kirg kirg commented Jul 6, 2017

No description provided.

@kirg kirg requested review from thuningxu and datoug July 6, 2017 23:05
@coveralls
Copy link

coveralls commented Jul 6, 2017

Coverage Status

Coverage increased (+0.1%) to 68.452% when pulling 7914e25 on fix-cli-defaults into 51b6724 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 68.432% when pulling baa6b9d on fix-cli-defaults into 51b6724 on master.

@datoug
Copy link
Contributor

datoug commented Jul 7, 2017

We probably should just deprecate these commands, which are duplicates from cherami-cli. I don't see the value of having the same commands in two tools, and they're causing code duplicates like this.

Or as @thuningxu has mentioned before, we can fold these two tools into a single binary, and only expose the 'admin' functionalities to admins. Although I personally slightly prefer the current solution.


"github.com/codegangsta/cli"
"github.com/uber/cherami-server/common"
"github.com/uber/cherami-server/tools/admin"
com "github.com/uber/cherami-server/tools/common"
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to toolscommon for better readability

@@ -123,7 +122,7 @@ func main() {
},
cli.StringSliceFlag{
Name: "zone_config, zc",
Usage: "Zone configs for multi_zone destinations. Format for each zone should be \"ZoneName,AllowPublish,AllowConsume,ReplicaCount\". For example: \"zone1,true,true,3\"",
Usage: "Zone configs for multi_zone destinations. Format for each zone should be \"Zone,AllowPublish,AllowConsume,ReplicaCount\". Ex: \"sjc1a,true,true,3\"",
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't want to use 'sjc1' in the open source repo

usageCGSkipOlderMessagesInSeconds = `Skip messages older than this duration, in seconds ('0' to skip none)`
usageCGDelaySeconds = `Delay, in seconds, to defer all messages by`
usageCGOwnerEmail = "Owner email"
usageCGZoneConfig = "Zone configs for multi-zone CG. For each zone, specify \"Zone,PreferedActiveZone\"; ex: \"dca1a,false\""
Copy link
Contributor

Choose a reason for hiding this comment

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

no 'dca1' or 'sjc1' in the open source repo

usageCGSkipOlderMessagesInSeconds = `Skip messages older than this duration, in seconds ('0' to skip none)`
usageCGDelaySeconds = `Delay, in seconds, to defer all messages by`
usageCGOwnerEmail = "Owner email"
usageCGZoneConfig = "Zone configs for multi-zone CG. For each zone, specify \"Zone,PreferedActiveZone\"; ex: \"dca1a,false\""
Copy link
Contributor

Choose a reason for hiding this comment

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

no dca1

Copy link
Contributor

Choose a reason for hiding this comment

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

still has dca1 here

// DefaultUnconsumedMessagesRetention is the default value for unconsumed messages retention
DefaultUnconsumedMessagesRetention = 7200
// DefaultConsumedMessagesRetention is the default value for consumed messages retention
DefaultConsumedMessagesRetention = 3600
Copy link
Contributor

Choose a reason for hiding this comment

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

these values are too low IMO. I suggest to change to 1d and 3d

@kirg
Copy link
Contributor Author

kirg commented Jul 7, 2017

About admin vs cli . I think Kobe is working on something to that effect. So not dealing with it in this diff.

@coveralls
Copy link

coveralls commented Jul 7, 2017

Coverage Status

Coverage increased (+0.2%) to 68.538% when pulling 9f1ba97 on fix-cli-defaults into 51b6724 on master.

@kobeyang
Copy link
Contributor

kobeyang commented Jul 7, 2017

Yes, I'm working on refactoring the cherami-cli and cherami-admin code. Will send out a PR later.

@coveralls
Copy link

coveralls commented Jul 7, 2017

Coverage Status

Coverage increased (+0.2%) to 68.493% when pulling e34e823 on fix-cli-defaults into 51b6724 on master.

@kirg kirg merged commit 61b6f33 into master Jul 7, 2017
datoug pushed a commit that referenced this pull request Jul 10, 2017
* CLI: fix defaults

* fix admin cli

* fix owner email

* cr feedback

* fix zone
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants