Skip to content

Example PR: Add config variable. #30

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

Closed
wants to merge 1 commit into from
Closed

Conversation

WilmsJochen
Copy link
Contributor

No description provided.

Copy link

PR review

‼️ It seems that you can still improve the quality of your PR. Have a look into this:

❌ Env variables in Helm chart

Adding environment variables to the Helm chart is crucial for maintaining configuration consistency and ease of deployment across different environments. This practice ensures that configuration changes can be managed centrally and applied uniformly.

To address the feedback, you need to add the EXAMPLE_CONFIG environment variable to the values.yaml file under the configmap section. This will ensure that the variable is properly injected into your application during deployment.

Here's how you can do this:

File: values.yaml

configmap:
  EXAMPLE_CONFIG: "your-config-value"

Make sure to replace "your-config-value" with the actual value you need for EXAMPLE_CONFIG. This change will help maintain best practices and improve the manageability of your application configuration.

❌ Env variables in default_env file

Ensuring that environment variables are listed in a default_env file is crucial for maintaining consistency and ease of configuration across different environments. This practice helps developers understand what environment variables are required and provides default values that can be overridden as needed. It also aids in avoiding hardcoding values directly in the code, which can lead to security issues and difficulties in managing configurations.

Here's how you can make the code changes to improve the code:

  1. Add the environment variable to the default_env file.
  2. Reference the environment variable in your code.

Filename: default_env

EXAMPLE_CONFIG=default_value

Filename: demo-microservice/src/services/exampleService.js

const exampleConfig = process.env.EXAMPLE_CONFIG || 'default_value';

By following these steps, you ensure that the EXAMPLE_CONFIG environment variable is documented and can be easily managed across different environments.

sidenote:

✅ Deployment & testing pipelines
❌ Docker file structure:
   Your Dockerfile is missing from the repository. Ensure it is placed at the root level of the project. Additionally, follow the provided example closely, including using multiple build stages, not running as root, and maintaining the order of commands.
✅ File structure based on example repo
✅ Usage of proper tests [Unit tests, Postman etc.]
❌ General coding guidelines:
   - Updated Readme File:
      The README.md file is incomplete. It should contain setup instructions and how to run the application.
   - Use Retries for External APIs:
      No evidence of retries in API calls. Implement retry logic for external API calls.
⚠️ This repo is currently not following company guidelines.

You might want to look into this so I prepared some PR's to bring it up to date.

Generated by Firstmate to make sure you can focus on coding new features.

@wvl94 wvl94 changed the title Add config variable. Example PR: Add config variable. Sep 12, 2024
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.

1 participant