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

Migrate push.bzl to attr.output #1263

Closed
wants to merge 1 commit into from
Closed

Migrate push.bzl to attr.output #1263

wants to merge 1 commit into from

Conversation

or-shachar
Copy link
Contributor

This is to complete #987

Thanks @Monnoroch for pointing this out

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: or-shachar
To complete the pull request process, please assign nlopezgi
You can assign the PR to them by writing /assign @nlopezgi in a comment when ready.

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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@or-shachar or-shachar force-pushed the removed-outputs-push_bzl branch 3 times, most recently from bc8f329 to 6fda186 Compare November 7, 2019 08:31
@or-shachar or-shachar force-pushed the removed-outputs-push_bzl branch from 6fda186 to ef341e9 Compare November 7, 2019 08:39
nlopezgi
nlopezgi reviewed Nov 7, 2019
View reviewed changes
Copy link
Contributor

@nlopezgi nlopezgi left a comment

Choose a reason for hiding this comment

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

Hi @or-shachar
Thanks for contributing.
Do you know if there are any other parts in our rules that fail with this flag?
A couple of nits,

container/push.bzl
attrs = dicts.add({
"digest_output": attr.output(),
Copy link
Contributor

Choose a reason for hiding this comment

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

add doc + add mandatory = True

container/push.bzl
@@ -131,9 +131,9 @@ def _impl(ctx):
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this now not also need to also return an
OutputGroupInfo(digest = depset([digest_output])
as indicated in bazelbuild/bazel#7977

@nlopezgi
Copy link
Contributor

nlopezgi commented Jan 8, 2020

Hi @or-shachar , will you be finishing work on this PR or should we close?

@nlopezgi nlopezgi closed this Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nlopezgi nlopezgi nlopezgi left review comments

@alex1545 alex1545 Awaiting requested review from alex1545

@smukherj1 smukherj1 Awaiting requested review from smukherj1 smukherj1 is a code owner

Assignees
No one assigned
Labels
cla: yes size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@or-shachar @k8s-ci-robot @nlopezgi @googlebot