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

226 - add ns to env vars when workload ns != service ns #227

Merged
merged 6 commits into from
May 19, 2023

Conversation

garyburgmann
Copy link
Contributor

@garyburgmann garyburgmann commented Mar 30, 2023

This PR is to add namespace to the env variable for the services which are in not in workload's namespace and defined via KubernetesLocalProcessConfig.yaml.

@garyburgmann garyburgmann requested review from a team, Tatsinnit and sabbour as code owners March 30, 2023 05:11
@garyburgmann
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Go1"

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

@garyburgmann Thanks for your PR!

My review of this pull request is as follows:

This pull request adds the ability to add namespace to environment variables when the workload namespace is not the same as the service namespace. This is a useful feature as it allows for cross-talk between namespaces.

The changes look good and the code is well-structured and easy to read. The tests are also comprehensive and cover all the scenarios.

However, I suggest adding a comment to the code to explain why the namespace is being added to the environment variables. This will help other developers understand the purpose of the code and make it easier to maintain in the future.

Overall, this is a good pull request and I recommend merging it.

@garyburgmann
Copy link
Contributor Author

built out the CLI locally last week and worked as intended

image

Copy link
Collaborator

@hsubramanianaks hsubramanianaks 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 to me @garyburgmann. We will test it and confirm.

@cxznmhdcxz
Copy link
Member

Passed on all 3 platforms.

@cxznmhdcxz cxznmhdcxz merged commit a2df828 into Azure:main May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Environment variables across namespaces
3 participants