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

Bug 1536629 - Send job msg immediately as job starts. To set initial JobState #671

Merged

Conversation

maleck13
Copy link
Contributor

@maleck13 maleck13 commented Jan 20, 2018

Changes proposed in this pull request

  • removes the need to explicitly set state in broker after starting new job

Which issue this PR fixes (This will close that issue when PR gets merged)
fixes 504

@eriknelson I believe this change will stop the need to set the state in the broker and fix #504

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 20, 2018
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 5f8903d on maleck13:504-remove-need-for-setting-state-broker into ** on openshift:master**.

@rthallisey
Copy link
Contributor

rthallisey commented Jan 23, 2018

@maleck13 Does this need to go in before #619?

@maleck13
Copy link
Contributor Author

@rthallisey yeah I think that makes sense. It is a small and distinct piece of work so can prob be merged faster then rebased onto the larger PR

@maleck13 maleck13 force-pushed the 504-remove-need-for-setting-state-broker branch from 38c0364 to 7b3035c Compare January 24, 2018 09:45
@@ -21,6 +21,7 @@
package broker

import (
"github.com/apex/log"
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like github.com/apex snuck in there again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sigh I think it must be my goimports that auto runs. :(

@maleck13 maleck13 force-pushed the 504-remove-need-for-setting-state-broker branch from 7b3035c to 8948bc7 Compare January 24, 2018 18:11
Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

LGTM

@shawn-hurley
Copy link
Contributor

I think this needs a Bugzilla to merge.

@rthallisey rthallisey added the 3.9 | release-1.1 Kubernetes 1.9 | Openshift 3.9 | Broker release-1.1 label Jan 24, 2018
@maleck13
Copy link
Contributor Author

Is the Bugzilla something I need to do?

@jmrodri jmrodri changed the title Send job msg immediately as job starts. To set initial JobState Bug 1536629 - Send job msg immediately as job starts. To set initial JobState Jan 25, 2018
@jmrodri
Copy link
Contributor

jmrodri commented Jan 25, 2018

@maleck13 I already had a bug relating to job state. Reusing that one.

@jmrodri jmrodri merged commit 154ddf9 into openshift:master Jan 25, 2018
jmrodri added a commit that referenced this pull request Jan 25, 2018
…initial JobState (#671)"

This reverts commit 154ddf9.

The original PR is more of tech-debt than actual bug fix. We don't want
that in the 1.1 release (OpenShift 3.9).
eriknelson pushed a commit that referenced this pull request Jan 25, 2018
…initial JobState (#671)" (#686)

This reverts commit 154ddf9.

The original PR is more of tech-debt than actual bug fix. We don't want
that in the 1.1 release (OpenShift 3.9).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.9 | release-1.1 Kubernetes 1.9 | Openshift 3.9 | Broker release-1.1 size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SetState (JobState) from within StartNewJob?
6 participants