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(ijwb): add environment variables to the run configuration #4138

Closed
wants to merge 6 commits into from

Conversation

JamyDev
Copy link
Contributor

@JamyDev JamyDev commented Nov 28, 2022

Checklist

  • I have filed an issue about this change and discussed potential changes with the maintainers.
  • I have received the approval from the maintainers to make this change.
  • This is not a stylistic, refactoring, or cleanup change.

Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See
the Contributions section in the README for more
details.

Discussion thread for this change

Issue number: #2042

Description of this change

This adds the option to set environment variables to your run configuration for any target type. For Test configurations --test_env should be used and thus this field is only shown when set to Run.

Reused the EnvironmentVariablesData that was used in the Python module and applied it everywhere by passing it as an argument to ScopedBlazeProcessHandler, which then passes it onto the ProcessGroup using .withEnvironment.

Since Go handles the Run Configuration differently, we make sure to add the environment variables there as well, in this case as the last one so the user can override variables set elsewhere.

Screenshots:

env_vars_new_1

(this was a go_binary target but I had to modify it for reasons, so it changed it to generic handler)

env_vars_new_2

env_vars_new_3

env_vars_new_py

@sgowroji sgowroji added product: IntelliJ IntelliJ plugin awaiting-maintainer Awaiting review from Bazel team on issues awaiting-review Awaiting review from Bazel team on PRs and removed awaiting-maintainer Awaiting review from Bazel team on issues labels Nov 29, 2022
@tpasternak
Copy link
Contributor

Hey @JamyDev, thank you for this PR! I have two general questions related to the code:

  1. I'm not sure if this was intended or not, but contrary to the linked issue Add support for setting Environment Variables via Run Configuration #2042, the java run configuration editor still does not contain the "Environment Variables" field, probably due to the fact that the initializeStates method is overridden in BlazeJavaRunConfigState.
    The implementation in BlazeCommandRunConfigurationCommonState.java has been extended by envVarsState, while BlazeJavaRunConfigState.java remains unchanged

  2. I don't fully understand the reason for which the getData() method is guarded by the "is supported command" check?

return ImmutableList.of(command, envVarsState, blazeFlags, exeFlags, blazeBinary);
}

/** @return The list of blaze flags that the user specified manually. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please update the comment to match the function return value.


private static final String ELEMENT_TAG = "env_state";

private EnvironmentVariablesData data =
EnvironmentVariablesData.create(ImmutableMap.of(), /* passParentEnvs= */ true);

private final BlazeCommandState command;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it will be simpler to store a boolean of whether the command is supported and check it when needed.

@@ -38,10 +38,6 @@ protected ImmutableList<RunConfigurationState> initializeStates() {
return ImmutableList.of(command, blazeFlags, exeFlags, envVars, debugPortState, blazeBinary);
}

EnvironmentVariablesState getEnvVarsState() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure why this method is removed here and in BlazePyRunConfigState.java

@mai93
Copy link
Collaborator

mai93 commented Jan 10, 2023

Thanks @JamyDev!

I also was not able to see the environment variables for java_binary target run configuration, I'm trying with this example project.

@JamyDev
Copy link
Contributor Author

JamyDev commented Jan 25, 2023

Thanks for the comments, I'll address them sometime soon.

@mai93 mai93 added awaiting-user-response Awaiting response from author on PRs and removed awaiting-review Awaiting review from Bazel team on PRs labels Mar 8, 2023
@mai93 mai93 assigned blorente and unassigned mai93 Apr 6, 2023
@github-actions
Copy link

github-actions bot commented Oct 7, 2023

Thank you for contributing to the IntelliJ repository! This pull request has been marked as stale since it has not had any activity in the last 6 months. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-review". Please reach out to the triage team (@bazelbuild/triage) if you think this PR is still relevant or you are interested in getting the PR merged.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Oct 7, 2023
@github-actions
Copy link

This pull request has been automatically closed due to inactivity. If you're still interested in pursuing this, please reach out to the triage team (@bazelbuild/triage). Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-user-response Awaiting response from author on PRs product: IntelliJ IntelliJ plugin stale Issues or PRs that are stale (no activity for 30 days)
Projects
Development

Successfully merging this pull request may close these issues.

5 participants