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

make watch job more efficient #70

Merged
merged 2 commits into from
Oct 5, 2020
Merged

make watch job more efficient #70

merged 2 commits into from
Oct 5, 2020

Conversation

toshok
Copy link
Contributor

@toshok toshok commented Oct 1, 2020

keep track of whether or not we have blocked jobs. this should let us know for certain when we're actually done, so we can stop waiting sooner.

We can stop and declare the build finished as soon as there are no running jobs (minus the watch job itself) and no blocked jobs.

Simplify the loop inside evalWorkflow to have no early returns, and just set another flag - anyRunning, which is true for jobs in either the queued or running circleci states.

The loop inside waitCircle continues to confound and is next on the list of places we should simplify, imo.

… recognize when we're definitely done (no blocked or running jobs)
@toshok toshok force-pushed the toshok.more-efficient-watch branch from d5efd7d to 007c1ec Compare October 2, 2020 13:52
for _, job := range wfJobs {
// always count ourself as finished so we don't wait for ourself
// skip ourself so we don't wait if we're the only job running
if job.Name == jobName {
continue
}

switch job.Status {
Copy link
Member

Choose a reason for hiding this comment

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

should we introduce a default stanza here to deal with unknown CircleCI states in case they introduce a new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah good call. hm, what should that stanza actually do though? ideally we'd include it in instrumentation, so we'd need to bubble it up someplace where we can send it.

Copy link
Member

Choose a reason for hiding this comment

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

treat it as a failure state, I'd say?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that worries me, anyone using circleci + buildevents would see failed builds if the watch job turns red, as opposed to broken traces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe treat it such that we don't early exit and run through the 2 minute "we think we might be done, sleeping and trying again" loop in the hopes that it'll change to a state we recognize, and also send it up in instrumentation? we can run a trigger against our own builds that would surface it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

going to address this in another PR so we have more space/time to think about it 👍

@toshok toshok merged commit 3813fb8 into main Oct 5, 2020
@toshok toshok deleted the toshok.more-efficient-watch branch October 5, 2020 17:36
toshok pushed a commit to honeycombio/buildevents-orb that referenced this pull request Oct 5, 2020
We've had a couple of releases that didn't end up getting updated in `orb.yml`. Skip them and update directly to 0.4.11 to pull in the watch step improvement in honeycombio/buildevents#70
MikeGoldsmith added a commit that referenced this pull request Jun 21, 2021
…ite_env

* 'main' of github.com:honeycombio/buildevents: (40 commits)
  add apply labels workflow (#93)
  GitLab CI example (#90)
  Build an arm64 release (#91)
  * publish a draft release to GitHub (#87)
  Prepare for 0.5.0 release, add changelog (#86)
  Add support for overriding default fields (#76)
  fix providerAzurePipelines in main.go (#84)
  add bitbucket support (#85)
  Ensure PRs from forks can run CI checks (#82)
  added quiet option to cmd (#80)
  Use public CircleCI context for build secrets (#77)
  add codeowners with integration-team (#78)
  Encode spaces in dataset names for the URL (#73)
  Add beeline-style propagation of the cmd context to called commands (#74)
  we're on circle for this, no need to build on travis any longer (#69)
  make watch job more efficient (#70)
  drop vendor/ and upgrade go to 1.15.2 (#68)
  add GitHub actions as a supported CI provider, with data from env vars
  Correct a download link
  Add support for Azure Piplines and some initial env vars
  ...

# Conflicts:
#	README.md
#	cmd_root.go
#	common.go
#	main.go
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.

2 participants