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

feat(*_image): allow setting env vars #2031

Merged
merged 2 commits into from
May 20, 2022
Merged

Conversation

sxlijin
Copy link
Contributor

@sxlijin sxlijin commented Feb 25, 2022

Introduce an env parameter in every *_image rule, which is a dict
whose keys and values will become environment variables in the resulting
Docker image.

Apply consistent formatting to every rule's docstring (args in the
docstring are now listed in the same order as in the method declaration)
as well as the container structure tests (metadataTest keys are now
sorted alphabetically).

Aside: the docstring formatting seems weird to me, but I'm sticking with the more common one in the repo.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

It is not possible to wire environment variables through to a *_image instance; they are forwarded to the env of the underlying *_binary (usually).

What is the new behavior?

It is now possible to wire environment variables through to a *_image.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@google-cla
Copy link

google-cla bot commented Feb 25, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@gravypod
Copy link
Collaborator

Change looks good. Could you add a test and sign the CLA? Thanks!

@sxlijin sxlijin changed the title feat(cc_image): allow setting env vars feat(*_image): allow setting env vars Mar 11, 2022
@sxlijin sxlijin marked this pull request as ready for review March 11, 2022 09:37
@sxlijin
Copy link
Contributor Author

sxlijin commented Mar 11, 2022

Finished the PR and added tests.

d_image doesn't have tests, but it looks like rules_d is still broken tracing the issue history so I didn't dive deep into that.

I need to fiddle with some stuff on my side for the CLA: I have a weird email setup.

Introduce an `env` parameter in every `*_image` rule, which is a dict
whose keys and values will become environment variables in the resulting
Docker image.

Apply consistent formatting to every rule's docstring (args in the
docstring are now listed in the same order as in the method declaration)
as well as the container structure tests (`metadataTest` keys are now
sorted alphabetically).
@sxlijin
Copy link
Contributor Author

sxlijin commented Mar 16, 2022

@gravypod let me know when you can take a look at this.

@sxlijin
Copy link
Contributor Author

sxlijin commented Mar 30, 2022

Bump?

@alexeagle
Copy link
Collaborator

sorry, this repo is kinda dead right now, see #2038

@sxlijin
Copy link
Contributor Author

sxlijin commented May 19, 2022

@uhthomas @linzhp y'all seem to be the new maintainers, so could you TAL at this / put it on your backlog?

(I can't update the reviewer set.)

Copy link
Collaborator

@linzhp linzhp left a comment

Choose a reason for hiding this comment

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

LGTM except there is a debug statement that we may want to remove

java/image.bzl Outdated
@@ -169,6 +169,9 @@ jar_dep_layer = rule(
def _jar_app_layer_impl(ctx):
"""Appends the app layer with all remaining runfiles."""

for k in ctx.attr.env:
print("jar_app_layer", k, ctx.attr.env[k])
Copy link
Collaborator

Choose a reason for hiding this comment

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

debug statements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops! yes indeed, removed

@sxlijin sxlijin requested a review from uhthomas as a code owner May 20, 2022 01:46
@linzhp linzhp merged commit 1b04e44 into bazelbuild:master May 20, 2022
@sxlijin sxlijin deleted the patch-1 branch May 20, 2022 20:56
St0rmingBr4in pushed a commit to St0rmingBr4in/rules_docker that referenced this pull request Oct 17, 2022
* feat(*_image): allow setting env vars

Introduce an `env` parameter in every `*_image` rule, which is a dict
whose keys and values will become environment variables in the resulting
Docker image.

Apply consistent formatting to every rule's docstring (args in the
docstring are now listed in the same order as in the method declaration)
as well as the container structure tests (`metadataTest` keys are now
sorted alphabetically).
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.

4 participants