-
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
fix oc describe panic #9097
fix oc describe panic #9097
Conversation
PI-Victor
commented
May 31, 2016
- fixes https://bugzilla.redhat.com/show_bug.cgi?id=1340784
- remove slice index limitation for revision commit
@@ -1369,7 +1369,7 @@ func describeBuildTriggerCauses(causes []buildapi.BuildTriggerCause, out *tabwri | |||
func squashGitInfo(sourceRevision *buildapi.SourceRevision, out *tabwriter.Writer) { | |||
if sourceRevision != nil && sourceRevision.Git != nil { | |||
rev := sourceRevision.Git | |||
formatString(out, "Commit", fmt.Sprintf("%s (%s)", rev.Commit[:7], rev.Message)) | |||
formatString(out, "Commit", fmt.Sprintf("%s (%s)", rev.Commit, rev.Message)) |
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.
can you check the length instead of showing the long commit hash here?
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.
that was my first solution, but went with this(i was thinking since we show the entire message...) . sure, i can do that.
@mfojtik changed my mind a couple of times, but now it's fixed :) |
@@ -1369,7 +1369,11 @@ func describeBuildTriggerCauses(causes []buildapi.BuildTriggerCause, out *tabwri | |||
func squashGitInfo(sourceRevision *buildapi.SourceRevision, out *tabwriter.Writer) { | |||
if sourceRevision != nil && sourceRevision.Git != nil { | |||
rev := sourceRevision.Git | |||
formatString(out, "Commit", fmt.Sprintf("%s (%s)", rev.Commit[:7], rev.Message)) | |||
commit := "" | |||
if len(rev.Commit) > 6 { |
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.
is there case when the commit would be >0 but less than 7? If not, we should just check if the commit is not empty and in that case don't print it at all (or just say "empty" or "unknown").
* fixes https://bugzilla.redhat.com/show_bug.cgi?id=1340784 * fix checking length of commit hash
lgtm [merge] |
[Test]ing while waiting on the merge queue |
re[test] |
@deads2k thanks for the merge but can you try to merge this again, tests are really flaky? |
[merge] On Thu, Jun 2, 2016 at 2:56 PM, Ionut Palade [email protected]
Michal Fojtik <[email protected] [email protected]> |
anyone know what's going on with our merge queue, seems this keeps failing? |
[test] |
[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4460/) (Image: devenv-rhel7_4318) |
[test] |
Evaluated for origin test up to 9ade759 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4460/) |
@bparees seems tests are passing, can you please merge? |
[merge] again |
Evaluated for origin merge up to 9ade759 |
@deads2k i appreciate it, thanks! |