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

Set keywords on appropriate lifecycle events. #14857

Closed

Conversation

benjaminp
Copy link
Collaborator

@benjaminp benjaminp commented Feb 17, 2022

The docs for the PublishLifecycleEventRequest say that the notification_keywords field should be set if the build event is InvocationAttemptStarted or BuildEnqueued. However, Bazel did not conform to this spec.

@benjaminp benjaminp changed the title lifecycle notification keywords Set keywords on appropriate lifecycle events. Feb 17, 2022
@Yannic
Copy link
Contributor

Yannic commented Feb 21, 2022

cc @lfpino @ulfjack

@lfpino
Copy link
Contributor

lfpino commented Feb 22, 2022

@michaeledgar could you please take a look?

@gregestren gregestren added the team-Core Skyframe, bazel query, BEP, options parsing, bazelrc label Mar 7, 2022
@Yannic
Copy link
Contributor

Yannic commented Mar 23, 2022

@meisterT can you help merging this please?

cc @Wyverald I think we may want to cherry-pick this into 5.x

case BUILD_ENQUEUED:
case INVOCATION_ATTEMPT_STARTED:
builder.addAllNotificationKeywords(getKeywords());
break;
Copy link
Member

Choose a reason for hiding this comment

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

please add an explicit default: // fall through here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, though I assumed a break; would be preferred over a fall-through comment.

The docs for the PublishLifecycleEventRequest say that the notification_keywords field should be set if the build event is InvocationAttemptStarted or BuildEnqueued. However, Bazel did not conform to this spec.
@benjaminp benjaminp force-pushed the lifecycle-notification-keywords branch from 9d7ff28 to b05653b Compare April 5, 2022 05:04
@bazel-io bazel-io closed this in 0a2a43f Apr 19, 2022
@brentleyjones
Copy link
Contributor

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Apr 19, 2022
@benjaminp benjaminp deleted the lifecycle-notification-keywords branch April 19, 2022 22:02
@ckolli5
Copy link

ckolli5 commented Apr 21, 2022

@bazel-io fork 5.2.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Apr 21, 2022
ckolli5 added a commit that referenced this pull request May 10, 2022
The docs for the PublishLifecycleEventRequest say that the notification_keywords field should be set if the build event is InvocationAttemptStarted or BuildEnqueued. However, Bazel did not conform to this spec.

Closes #14857.

PiperOrigin-RevId: 442902471

Co-authored-by: Benjamin Peterson <[email protected]>
meteorcloudy pushed a commit that referenced this pull request May 10, 2022
The docs for the PublishLifecycleEventRequest say that the notification_keywords field should be set if the build event is InvocationAttemptStarted or BuildEnqueued. However, Bazel did not conform to this spec.

Closes #14857.

PiperOrigin-RevId: 442902471

Co-authored-by: Benjamin Peterson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Core Skyframe, bazel query, BEP, options parsing, bazelrc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants