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

fix(task-processor): the extraEnv where added in the wrong place #220

Merged
merged 1 commit into from
May 21, 2024

Conversation

blackjid
Copy link
Contributor

@blackjid blackjid commented May 16, 2024

Thanks for submitting a PR! Please check the boxes below:

  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have bumped the version number in /charts/flagsmith/Chart.yaml in the section version or I'm merging to a
    release branch

Changes

Fix bug introduced in thhis pr #187

The environment variables added for the task processor should go inside the container that runs the task processor rather in the deployment template spec.

The task processor was already including the api environment so it was not necessary to add merge both extraEnv

Also the extraEnv keys and values were added as yaml (key: value), instead of the expected object with the name and value keys.

How did you test this code?

I test it manually. With a values

api:
  extraEnv:
    ALLOW_REGISTRATION_WITHOUT_INVITE: false

taskProcessor:
  enabled: true
  extraEnv:
    foo: bar

The expected result in the api deployment, inside the flagsmith-task-processor container

env:
....
- name: ALLOW_REGISTRATION_WITHOUT_INVITE
  value: "false"
...

and in the task processor deployment, inside the flagsmith-task-processor container

env:
....
- name: ALLOW_REGISTRATION_WITHOUT_INVITE
  value: "false"
- name: foo
  value: bar
...

The environment variables added for the task processor should go inside
the container that runs the task processor rather in the deployment
template spec.

The task processor was already including the api environment so it was
not necessary to add merge both `extraEnv`

Also the `extraEnv` keys and values were added as yaml, instead of the
expected object with the `name` and `value` keys.
@blackjid blackjid requested a review from a team as a code owner May 16, 2024 19:48
@blackjid blackjid requested review from zachaysan and removed request for a team May 16, 2024 19:48
blackjid referenced this pull request May 16, 2024
* Add extraEnv for taskProcessor service

* updated chart version

* added logic to support extraEnv

* added env

---------

Co-authored-by: Gagan <[email protected]>
@zachaysan zachaysan requested a review from khvn26 May 16, 2024 21:34
Copy link

@zachaysan zachaysan left a comment

Choose a reason for hiding this comment

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

The changes look reasonable to me, but I lack experience with this repository so I've tagged @khvn26 for a review that would come with an approval stamp on this PR since he knows more about this repository than I do.

Thanks for contributing!

@khvn26 khvn26 merged commit 19b8cb6 into Flagsmith:main May 21, 2024
2 checks passed
@blackjid blackjid deleted the fix_task_processor_extra_envs branch May 21, 2024 23:31
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.

None yet

3 participants