Add support for env_inherit

[ttpathan]

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Adds support for env_inherit attribute in go_test.
This is useful for us for integration tests, where we pass database info in CI as environment variables.
env_inherit is supported for native Bazel rules.

Which issues(s) does this PR fix?

Closes #3241

Other notes for review

Inherited environment requires Bazel 5.2.0+. We only pass inherited_environment if env_inherit has been explicitly specified, so that rules_go still remains compatible with its minimum supported Bazel version.

I wasn't quite sure on how to add a test for this. I need to be able to pass in an environment variable from the shell env, but there didn't seem like anything existing set up to do this - it just runs test targets directly, with no wrapper(?). Would appreciate advice on how to approach that, thanks!

[fmeum]

Thanks for the contribution, I will take a look.

You can create proper integration tests using go_bazel_test, see https://github.com/bazelbuild/rules_go/blob/master/tests/core/go_binary/non_executable_test.go for an example. In this way, you can set environment variables, then execute Bazel and make assertions about the outcome.

[fmeum]

@achew22 Are you ultimately fine with this approach or would you prefer an explicit Bazel version check with a more descriptive error message?

[achew22]

Sorry to be so slow on response to this one. I would definitely prefer the explicit check. My bias on these checks is to see if the property or attribute exists on the object. If that's not possible because it's something like an argument to a function, then a version check is sufficient. Also, please include a comment indicating why the version check is there and a link so that later we can reevaluate if the check is still necessary.

Again, sorry to be so slow!

[ttpathan]

Looks like CI is failing because it uses 4.2.1:
https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/018244d5-106f-4757-ac4e-7bbe108686a7/tests/core/go_test/env_inherit_test/test.log

[fmeum]

@achew22 What do you think, could we temporarily include this without test coverage until we raise the minimum to 5.3.0?

[ttpathan]

No worries at all for the delay.

I tried adding an explicit version check, assuming native.bazel_version would work, but turns out this is only available for workspace macros? bazelbuild/bazel#8305. Is there a better way to check this?

What do you think, could we temporarily include this without test coverage until we raise the minimum to 5.3.0?

Its only the "ubuntu1804_bazel400" job which fails, the rest are fine (because I assume they use latest Bazel?). Maybe I should just ignore the test case in that specific CI job? https://github.com/bazelbuild/rules_go/blob/b4292b15a157158fac95069fd988b64c24fa16e3/.bazelci/presubmit.yml#L10-L11

[fmeum]

I tried adding an explicit version check, assuming native.bazel_version would work, but turns out this is only available for workspace macros? bazelbuild/bazel#8305. Is there a better way to check this?

No, unfortunately not. We have to add a repository rule exposing the version as a Starlark constant and can then use the bazel-skylib helper functions to assert the minimum in the rule.

[ttpathan]

Sorry, this is all very new to me, unsure if I've done it correctly - let me know if anything needs changing.
Not entirely sure on how to resolve the BCR test module job, I assume I need some sort of use_repo somewhere.
The error that is shown is visible in https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/0182f3a1-6e90-414c-8853-7c1947b8097f/tests/core/go_test/env_inherit_test/test.log. I can also go ahead and remove the target from testing in ubuntu1804_bazel400

[fmeum]

@linzhp Now that we are on 5.1.0, do you think we can go up to 5.2.0 in the next release? That would greatly simplify this PR.

[linzhp]

I am OK with that

[fmeum]

@ttpathan Sorry for the back and forth (and delay), but we are finally ready to update to 5.2.0. Instead of the version check, you can just use inherited_environment unconditionally and write a simple test.

[fmeum]

@ttpathan With Bazel 6 out, there is really nothing preventing us from updating to 5.2.0 unconditionally now. Are you still interested in getting this merged? We could now do so without the version check.

[fmeum]

@ttpathan I pushed a new commit that enables the feature unconditionally and increases the minimum required Bazel version. I kept you as the author. Thanks for the contribution and sorry for the long delay!