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.

Sign up for GitHub

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

Jump to bottom

only set the build timestamps exactly once #15483

Merged
merged 1 commit into from Jul 28, 2017
Merged

only set the build timestamps exactly once #15483

merged 1 commit into from Jul 28, 2017

Conversation

bparees
Copy link
Contributor

@bparees bparees commented Jul 26, 2017 •
edited

@openshift-merge-robot openshift-merge-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 26, 2017
@bparees
Copy link
Contributor Author

bparees commented Jul 26, 2017 •
edited

@csrwng ptal
/test extended_builds

@bparees bparees assigned csrwng and unassigned mfojtik and smarterclayton Jul 26, 2017
@bparees
Copy link
Contributor Author

bparees commented Jul 26, 2017

hm. need to rework how starttime is handled still.

@bparees bparees force-pushed the logsnippet_update branch 2 times, most recently from 0f3146e to f3d5ce1 Compare July 26, 2017 22:17
@bparees
Copy link
Contributor Author

bparees commented Jul 26, 2017

@csrwng ok now it should be ready for review.

@bparees
Copy link
Contributor Author

bparees commented Jul 26, 2017

/test extended_builds

@csrwng
Copy link
Contributor

csrwng commented Jul 26, 2017

@bparees so now we set the log snippet even if the build is not failed. Is that ok?

@bparees
Copy link
Contributor Author

bparees commented Jul 26, 2017

@bparees so now we set the log snippet even if the build is not failed. Is that ok?

hm.. well we won't find a log snippet to set unless the pod failed, but i should probably make the check better.

@bparees bparees force-pushed the logsnippet_update branch 2 times, most recently from 313d341 to 6aaa9eb Compare July 26, 2017 22:45
@bparees
Copy link
Contributor Author

bparees commented Jul 26, 2017

@csrwng updated.

@bparees
Copy link
Contributor Author

bparees commented Jul 26, 2017

note that the idea here is to rely on update.IsEmpty() now.

@csrwng
Copy link
Contributor

csrwng commented Jul 26, 2017

note that the idea here is to rely on update.IsEmpty() now.

You should also check whether LogSnippet is empty before setting it.

@bparees
Copy link
Contributor Author

bparees commented Jul 26, 2017 

 You should also check whether LogSnippet is empty before setting it.

good catch. done.

@openshift-merge-robot openshift-merge-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 26, 2017
@csrwng
Copy link
Contributor

csrwng commented Jul 26, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2017
@csrwng
Copy link
Contributor

csrwng commented Jul 26, 2017

/approve no-issue

@smarterclayton
Copy link
Contributor

A follow up for a test that verifies builds only write once ina variety of scenarios would be appreciated.

@smarterclayton
Copy link
Contributor

Link to release 3.6 pr here please.

@bparees
Copy link
Contributor Author

bparees commented Jul 27, 2017

incidentally our shouldIgnore check on handleBuild should already be keeping us out of a lot of this code (any build that's failed, with a timestamp, and with a logsnippet, will be ignored) so it's a bit of belt+suspenders.

@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2017
@bparees bparees added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2017
@bparees
Copy link
Contributor Author

bparees commented Jul 27, 2017

lgtm re-added, only change was to the tests.

@bparees
Copy link
Contributor Author

bparees commented Jul 27, 2017

/test extended_builds

@bparees
Copy link
Contributor Author

bparees commented Jul 27, 2017

backport PR: #15487

@bparees
Copy link
Contributor Author

bparees commented Jul 27, 2017

/retest

@bparees
Copy link
Contributor Author

bparees commented Jul 27, 2017

/approve no-issue

@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, csrwng

Associated issue requirement bypassed by: bparees

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2017
@bparees
Copy link
Contributor Author

bparees commented Jul 27, 2017

/retest

1 similar comment
@bparees
Copy link
Contributor Author

bparees commented Jul 27, 2017

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 15447, 15411, 15483, 15478, 15481)

@openshift-merge-robot openshift-merge-robot merged commit 728cf0b into openshift:master Jul 28, 2017
@bparees bparees deleted the logsnippet_update branch July 31, 2017 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@csrwng csrwng

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@bparees @csrwng @smarterclayton @openshift-merge-robot @mfojtik @openshift-ci-robot