-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
status: Report tls routes with unspecified termination type #6078
status: Report tls routes with unspecified termination type #6078
Conversation
I think this is the straw that breaks the camel's back. We need to provide advice for how to resolve these problems in a structured way. This particular one is valuable because there are only a few reasonable options for fix this problem. See #4189 Can you start adding the structure here? The marker would need an Even its just displayed on a separate line in the default view, it would be a starting point for us to start fixing up the markers. |
Sure thing |
@deads2k this is ready for review. |
}, | ||
} | ||
|
||
cmd.Flags().StringVarP(&outputFormat, "output", "o", outputFormat, "Output format. One of: dot.") | ||
cmd.Flags().StringVarP(&opts.outputFormat, "output", "o", opts.outputFormat, "Output format. One of: dot.") | ||
cmd.Flags().BoolVar(&opts.suggest, "suggest", opts.suggest, "Make suggestions for possible warnings/errors.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabianofranz thoughts on a the flag name?
@Kargakis "Get automated suggestions for resolving errors and warnings"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get automated suggestions for resolving errors and warnings
Better than what I had, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--suggest
sounds good to me. Other ideas: --hints
, --tips
. We could use something more generic if we plan to add things other than suggestions in future, like --verbose
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the final format is
* this is the error
fix: this is how you fix it
then another idea is --fix
or --fixes
or --suggested-fixes
.
If there are suggestible errors, we may want a message to hint about the flag in the end, e.g. |
How excessive are the fix messages going to be? On Mon, Nov 30, 2015 at 1:35 PM, Fabiano Franz notifications@github.com
|
Hard to know. I'm sure it will vary by person and by problem. Are you considering whether we should have it on by default or always have it on? |
As a for instance. @Kargakis suggested |
Hrm - if a user must fix it, they need to know how to fix it. So some If we think a user should always fix these, we should default the flag to on On Nov 30, 2015, at 2:55 PM, David Eads notifications@github.com wrote: Hard to know. I'm sure it will vary by person and by problem. Are you As a for instance. @Kargakis https://github.com/kargakis suggested oc edit. — |
@deads2k @smarterclayton switched to one oc patch command and providing the supported termination types here. Generally I agree with @deads2k that it's hard to say how excessive the fixes will be. It seems that one line is not enough... |
Another thing is there are cases where we will display info level messages like "your istag is missing but a build config is pointing to it" and be able to provide suggestions like "start this build". We shouldn't prefix then with "fix: ", should we? How about "tip: " or "hint: " ? |
I don't like either tip or hint because they are too informal or too likely On Dec 1, 2015, at 3:41 PM, Michail Kargakis notifications@github.com Another thing is there are cases where we will display info level messages — |
I like try since it replaces the first verb in the suggestion sentence plus it's small. Currently:
|
How about using two lines for the suggestion? One where we suggest the fix, and optionally one more that describes any changes that need to happen in the command to be properly functional (like what I am doing above) ? |
I like "try:" and I think its clear enough on the one line. Everyone happy with the flag |
Note acronyms (tls) should ALWAYS be capitalized (TLS) On Dec 2, 2015, at 5:38 AM, Michail Kargakis notifications@github.com I like try since it replaces the first verb in the suggestion sentence plus [vagrant@localhost sample-app]$ oc create -f [vagrant@localhost sample-app]$ oc status svc/doesntmatter - 172.30.173.91:5432 -> 8080 Warnings:
There are some suggestions for the info/warnings/errors listed above. To see more, use 'oc describe /'. [vagrant@localhost sample-app]$ oc status --suggest svc/doesntmatter - 172.30.173.91:5432 -> 8080 Warnings:
To see more, use 'oc describe /'. — |
code lgtm. Waiting before merge to allow later risers a chance to comment on flag name and text. |
(updated to TLS) |
Is there a corresponding issue already for the web console? Edit: Opened #6162 |
Hold on merge, I have a few comments about this in general. On Dec 2, 2015, at 8:16 AM, David Eads notifications@github.com wrote: code lgtm. Waiting before merge to allow later risers a chance to comment on flag name — |
I'm no longer convinced we want to show all suggestions by default. I think we should display the count of suggestions and fixes at the end and make it obvious, then instruct to use --suggest or -v (verbose) to get the details. We should not show the "oc describe" line in this case. If the user specifies -v, show both suggestions and fixes. This gives us more room for items. I would want the count to say "warning: 3 issues identified, use -v to see suggestions" |
We already use warnings for warning level messages. Any other more appropriate word here? |
Aren't these warnings? On Thu, Dec 3, 2015 at 3:27 PM, Michail Kargakis notifications@github.com
|
If we have |
Should I change it to --verbose or use only the shorthand? |
Well, I've never wanted to type |
Let's just remove --suggest. -v is what users expect. On Dec 9, 2015, at 1:54 PM, David Eads notifications@github.com wrote: Should I change it to --verbose or use only the shorthand? Well, I've never wanted to type --verbose. Take your pick, I don't feel |
Removed |
if errorMarkers := allMarkers.BySeverity(osgraph.ErrorSeverity); len(errorMarkers) > 0 { | ||
fmt.Fprintln(out, "Errors:") | ||
for _, marker := range errorMarkers { | ||
errors++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors = len(errorMarkers)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knew something was wrong with this.
Updated as per latest comments
|
lgtm [merge] |
Updated our tests to run in verbose mode. |
#6259 flake re[merge] |
@bparees @csrwng have you seen this before?
|
[merge] |
[Test]ing while waiting on the merge queue |
@Kargakis @deads2k Seems like an issue initializing the default cluster policy?
|
Evaluated for origin test up to 73597be |
I think I opened a test-flake issue for this right around 3.1 On Thu, Dec 10, 2015 at 1:11 PM, David Eads notifications@github.com
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7720/) |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4289/) (Image: devenv-rhel7_2922) |
We need to bump the timeout for this test. Opened #6262 [merge] |
Evaluated for origin merge up to 73597be |
…ion-in-tls-config Merged by openshift-bot
Forked from #6076 (comment)
Fixes #4189
@deads2k @smarterclayton