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

feat: add support for arbitrary headers rctx.download[_and_extract] #19501

Closed
wants to merge 18 commits into from
Closed

feat: add support for arbitrary headers rctx.download[_and_extract] #19501

wants to merge 18 commits into from

Conversation

thesayyn
Copy link
Contributor

@thesayyn thesayyn commented Sep 12, 2023 •
edited
Loading

fixes #17829

@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. labels Sep 12, 2023
@thesayyn thesayyn mentioned this pull request Sep 12, 2023
Closed
@thesayyn
Copy link
Contributor Author

@fmeum this is not quite what i want. Even with change, if you are using credential helpers, or some other credential factory, the headers disappear which doesn't solve the problem we are having. I want these headers to be set unconditionally for all URLs. credential helpers might override them but should not make the disappear.

Would that still be acceptable here or should i go through an RFC process first?

@thesayyn thesayyn requested a review from a team as a code owner October 25, 2023 21:35
@thesayyn thesayyn requested a review from fmeum October 25, 2023 21:36
@fmeum
Copy link
Collaborator

fmeum commented Oct 29, 2023

I don't know much about credential helpers. @Yannic Could you provide advice here? How should a credential helper interact with headers specified in Starlark?

@tjgq
Copy link
Contributor

tjgq commented Oct 29, 2023

I don't know much about credential helpers. @Yannic Could you provide advice here? How should a credential helper interact with headers specified in Starlark?

We should merge headers from the two sources. If the same header is provided from both sides, the value from the credential helper should take precedence, matching the preexisting behavior for the Starlark auth attribute (see https://cs.opensource.google/bazel/bazel/+/master:src/test/shell/bazel/starlark_repository_test.sh;l=1817;drc=175a18dc9f58ba40e287336e06a26617a0becd0a).

Yannic
Yannic reviewed Oct 29, 2023
View reviewed changes
Copy link
Contributor

@Yannic Yannic left a comment

Choose a reason for hiding this comment

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

LGTM

...java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java Outdated Show resolved Hide resolved
...java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java Outdated Show resolved Hide resolved
...ava/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java Show resolved Hide resolved
@thesayyn
Copy link
Contributor Author

@fmeum this is ready I believe. After I get the okay from you, I'll add the tests.

fmeum
fmeum reviewed Nov 3, 2023
View reviewed changes
Copy link
Collaborator

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

This looks good to me with a few comments. Since @meteorcloudy and @tjgq seem to be on board, I think that you can go ahead with the tests.

...java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java Outdated Show resolved Hide resolved
...java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java Outdated Show resolved Hide resolved
...ava/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java Outdated Show resolved Hide resolved
@thesayyn
Copy link
Contributor Author

thesayyn commented Nov 8, 2023

This looks good to me with a few comments. Since @meteorcloudy and @tjgq seem to be on board, I think that you can go ahead with the tests.

Great, will get into that today.

@thesayyn
Copy link
Contributor Author

thesayyn commented Nov 24, 2023 •
edited
Loading

@fmeum this should be ready. I'll wait for the CI.

It would be sweet if we could get this cherry picked into 7.0

@fmeum
Copy link
Collaborator

fmeum commented Nov 24, 2023

@thesayyn There are still CI failures.

As far as I know the final RC for 7.0.0 is to be cut on Monday.

@meteorcloudy Could you already take a look?

meteorcloudy
meteorcloudy reviewed Nov 24, 2023
View reviewed changes
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Thanks, please fix the presubmit!

...ava/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java Outdated Show resolved Hide resolved
@thesayyn
Copy link
Contributor Author

There are still CI failures.

it should pass now, will carefully watch and fix it if it doesn't throughout the day.

@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 Dec 6, 2023
@meteorcloudy
Copy link
Member

@bazel-io fork 7.1.0

@bazel-io bazel-io mentioned this pull request Dec 6, 2023
Closed
@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 Dec 6, 2023
@meteorcloudy
Copy link
Member

@thesayyn Can you please resolve the merge conflicts?

tjgq
tjgq reviewed Dec 6, 2023
View reviewed changes
Copy link
Contributor

@tjgq tjgq left a comment

Choose a reason for hiding this comment

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

Looks good; do you mind rebasing to fix the merge conflict, and addressing my last two comments?

...ava/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java Outdated Show resolved Hide resolved
...ava/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java Outdated Show resolved Hide resolved
@thesayyn thesayyn force-pushed the arb_headers branch 2 times, most recently from 4b2879b to e2578f4 Compare December 6, 2023 23:15
tjgq
tjgq approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@tjgq tjgq left a comment

Choose a reason for hiding this comment

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

Thanks for going the extra mile and implementing the more complex dict value type :)

@tjgq tjgq added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Dec 6, 2023
@thesayyn
Copy link
Contributor Author

thesayyn commented Dec 7, 2023

Probably 7.1

Is there possibility that this could be released with 7.0? Since parallel downloads feature made it there it would be great to have this too.

@meteorcloudy
Copy link
Member

@thesayyn Sorry, Bazel 7 release has been lasting for more than a month, we already pushed back many feature cherry-picks to 7.1.0. We'll start preparing 7.1.0 shortly after Bazel 7 is out.

@copybara-service copybara-service bot closed this in 2b8885e Dec 7, 2023
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Dec 7, 2023
@tjgq
Copy link
Contributor

tjgq commented Dec 7, 2023

@bazel-io fork 7.1.0

@bazel-io bazel-io mentioned this pull request Dec 7, 2023
Closed
@thesayyn thesayyn mentioned this pull request Dec 11, 2023
Closed
@thesayyn thesayyn mentioned this pull request Jan 11, 2024
Closed
Wyverald pushed a commit that referenced this pull request Jan 22, 2024
@thesayyn @Wyverald
Add support for arbitrary headers to rctx.download[_and_extract]
f4e3302 
fixes #17829

Closes #19501.

PiperOrigin-RevId: 588750878
Change-Id: I11c982864135d8e1c4420099ce27f67accd3fe20
@Wyverald Wyverald mentioned this pull request Jan 22, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jan 22, 2024
@Wyverald @thesayyn
[7.1.0] Add support for arbitrary headers to rctx.download[_and_extra…
8edf5e5 
…ct] (#20979)

fixes #17829

Closes #19501.

PiperOrigin-RevId: 588750878
Change-Id: I11c982864135d8e1c4420099ce27f67accd3fe20

Co-authored-by: thesayyn <[email protected]>
Wyverald added a commit that referenced this pull request Jan 23, 2024
@Wyverald @thesayyn
[7.1.0] Add support for arbitrary headers to rctx.download[_and_extra…
2697e0c 
…ct] (#20979)

fixes #17829

Closes #19501.

PiperOrigin-RevId: 588750878
Change-Id: I11c982864135d8e1c4420099ce27f67accd3fe20

Co-authored-by: thesayyn <[email protected]>
@thesayyn thesayyn mentioned this pull request May 8, 2024
Closed
@axeluhlig axeluhlig mentioned this pull request Aug 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yannic Yannic Yannic left review comments

@fmeum fmeum fmeum left review comments

@tjgq tjgq tjgq approved these changes

@meteorcloudy meteorcloudy meteorcloudy approved these changes

Assignees
No one assigned
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow arbitrary headers to sent with Bazel Downloader
6 participants
@thesayyn @fmeum @tjgq @meteorcloudy @Yannic @bazel-io