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: Add Helm configMap to update application configuration #10157

Merged

Conversation

geekup-legodevops
Copy link
Contributor

Description

  • Adding Helm ConfigMap for users who deploy Appsmith by Helm Chart to change application configuration
  • Refactor the flow that being used to generate environment variables for supporting predefined environment variables when initializing the container

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@vercel
Copy link

vercel bot commented Jan 5, 2022

@geekup-legodevops is attempting to deploy a commit to the Appsmith Team on Vercel.

A member of the Team first needs to authorize it.

deploy/docker/templates/docker.env.sh Outdated Show resolved Hide resolved
deploy/docker/entrypoint.sh Outdated Show resolved Hide resolved
deploy/docker/entrypoint.sh Outdated Show resolved Hide resolved
deploy/docker/entrypoint.sh Outdated Show resolved Hide resolved
deploy/docker/entrypoint.sh Outdated Show resolved Hide resolved
deploy/helm/README.md Outdated Show resolved Hide resolved
deploy/helm/README.md Outdated Show resolved Hide resolved
deploy/helm/values.yaml Outdated Show resolved Hide resolved
@github-actions
Copy link

This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected.

@github-actions github-actions bot added the Stale label Jan 17, 2022
Copy link
Member

@sharat87 sharat87 left a comment

Choose a reason for hiding this comment

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

Looks much better, thanks guys. Just a couple of minor questions, otherwise we are ready to merge this in!

deploy/docker/entrypoint.sh Outdated Show resolved Hide resolved
deploy/docker/templates/docker.env.sh Outdated Show resolved Hide resolved
@sharat87 sharat87 changed the title Feature: Add Helm configMap to update application configuration feat: Add Helm configMap to update application configuration Jan 31, 2022
@sharat87 sharat87 merged commit e85b34d into appsmithorg:release Jan 31, 2022
echo 'Load environment configuration'
set -o allexport
. "$ENV_PATH"
. "$TEMPLATES_PATH/pre-define.env"

Choose a reason for hiding this comment

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

Hi all,
seems to me, this line, does not work as expcected on our kubernetes 1.21 environment.
only default-env-vars from $ENV_PATH are actually used within the script, following "pre-define.env"-files are not picked up, even though the file exists and contains values (e.g. for mongo-db).

See attached screenshot, where I entered some debugging points to point out env-variables while running entrypoint.sh:
error_on_override

Choose a reason for hiding this comment

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

@sharat87 could you have a look at this?

Copy link
Member

Choose a reason for hiding this comment

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

Hey, while I'm not sure of this without looking at the entrypoint_wdr.sh script, I think you are dealing with the issue fixed in this PR: #11799. We should be publishing it soon.

As a workaround in the meantime, could you try adding a backslash just before the ? and see if that makes a difference?

Choose a reason for hiding this comment

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

@sharat87 Thank you for pointing out #11799 to me. Actually escaping Question-Marks (?) and Ampersands (&) with a backslash (or in my case double back-slash to get it through our own scripting-pipeline) worked for me. Looking forward to the PR to be released, as other special-chars might be problematic as well (as described in the PR)
Thank you

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.

None yet

3 participants