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

Issue #2234 remove flags for metrics, logging and service catalog #2241

Merged
merged 1 commit into from
Apr 12, 2018

Conversation

gbraad
Copy link
Member

@gbraad gbraad commented Apr 4, 2018

See openshift/origin#19209 for more info
See #2248 for more info

/cc: @deads2k @jcantrill

@LalatenduMohanty LalatenduMohanty changed the title Issue #2234 remove flags for metrics and logging [Do not merge] Issue #2234 remove flags for metrics and logging Apr 4, 2018
@gbraad gbraad changed the title [Do not merge] Issue #2234 remove flags for metrics and logging Issue #2234 remove flags for metrics and logging Apr 6, 2018
@praveenkumar
Copy link
Contributor

@gbraad Since now your removed the flags so might be need to remove those things from the integration feature file also, currently CI is failing for same.

@@ -605,9 +605,6 @@ func initClusterUpFlags() *flag.FlagSet {
clusterUpFlagSet.AddFlag(cmdUtil.HttpsProxyFlag)

if minishiftConfig.EnableExperimental {
clusterUpFlagSet.Bool(configCmd.Metrics.Name, false, "Install metrics (experimental)")
clusterUpFlagSet.Bool(configCmd.Logging.Name, false, "Install logging (experimental)")
clusterUpFlagSet.Bool(configCmd.ServiceCatalog.Name, false, "Install service catalog (experimental)")
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to update the integration test case as well, as of now failing for CI.

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed

@gbraad gbraad changed the title Issue #2234 remove flags for metrics and logging Issue #2234 remove flags for metrics, logging and service catalog Apr 6, 2018
@@ -71,11 +71,8 @@ var (
HostPvDir = createConfigSetting("host-pv-dir", SetString, []setFn{validations.IsValidPath}, nil, true, nil)
ServerLogLevel = createConfigSetting("server-loglevel", SetInt, []setFn{validations.IsPositive}, nil, true, nil)
OpenshiftEnv = createConfigSetting("openshift-env", nil, nil, nil, false, nil)
Metrics = createConfigSetting("metrics", SetBool, nil, nil, true, nil)
Logging = createConfigSetting("logging", SetBool, nil, nil, true, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


// future enabled flags
ServiceCatalog = createConfigSetting("service-catalog", SetBool, nil, nil, true, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

| skip-registry-check | f | false |
| skip-registration | F | false |
| skip-registry-check | 0 | false |
| skip-registration | f | false |
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

| logging | TRuE |
| metrics | positive |
| metrics | yes |
| skip-registration | TRuE |
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@LalatenduMohanty
Copy link
Member

LalatenduMohanty commented Apr 12, 2018

@minishift/minishift-dev AFAIK Users need these features need these from Minishift. So IMO we need to provide them in future. May be after the oc cluster up restructure is done. I am not sure if we will provide add-ons or flags with minishift start for these feature at this point. Just wanted to make sure we agree to this before merging this.

@gbraad
Copy link
Member Author

gbraad commented Apr 12, 2018 via email

@LalatenduMohanty
Copy link
Member

Since they are experimental, it is possible for us to remove a feature when we see fit. At the moment we have to many issues resulting from this, and we have no support story.

@gbraad That was not my point. I am fine with removing it time being. But this needs to be fixed after oc cluster up restructure happens.

@gbraad
Copy link
Member Author

gbraad commented Apr 12, 2018 via email

@LalatenduMohanty
Copy link
Member

LalatenduMohanty commented Apr 12, 2018

I doubt if WE can ever fix this. it would better to solve this as an addon and pass the responsibility to the actual implementers.

I do not think we have any option here i.e. get away with not fixing it. But we will get the input from concerned people.

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.

None yet

5 participants