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.

Sign up for GitHub

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

Jump to bottom

Add the yaml env load #835

Merged
merged 1 commit into from
Sep 24, 2024
Merged

Add the yaml env load #835

merged 1 commit into from
Sep 24, 2024

Conversation

athiruma
Copy link
Collaborator

Type of change

Note: Fill x in []

  • bug
  • enhancement
  • documentation
  • dependencies

Description

Add environment variables load by yaml file.

For security reasons, all pull requests need to be approved first before running any automated CI

@athiruma athiruma added enhancement New feature or request ok-to-test PR ok to test labels Sep 12, 2024
@athiruma athiruma requested a review from ebattat September 12, 2024 11:40
@athiruma athiruma self-assigned this Sep 12, 2024
ebattat
ebattat reviewed Sep 12, 2024
View reviewed changes
Copy link
Collaborator

@ebattat ebattat left a comment

Choose a reason for hiding this comment

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

Great initiative !!!!
need to update the readme file how to run podman using -v yaml file

.gitignore Outdated
@@ -218,3 +218,4 @@ empty_test_environment_variables.py
/cloud_governance/policy/send_mail.py
cloudsensei/.env.txt
.vscode
cloud_governance/main/env.yaml
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where did u put this file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

inside the main folder

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I dont see it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added to .gitignore file that's why it is hidden.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@athiruma ,
cloud_governance/main/env.yaml: remove from .gitignore and it should be filled with default/empty values
cloud_governance/main/test_env.yaml: add to .gitignore and it should be filled with real values

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need to maintain 2 files, we can use test_env.yaml. In the readme, we will mention creating a copy of
cp test_env.yaml env_yaml and change appropriate values.

@athiruma athiruma force-pushed the add_env_yaml branch 3 times, most recently from 9e2f76f to 11092cd Compare September 13, 2024 10:39
ebattat
ebattat reviewed Sep 13, 2024
View reviewed changes
Copy link
Collaborator

@ebattat ebattat left a comment

Choose a reason for hiding this comment

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

Pls add to readme file how to run the container using env.yaml instead of passing environment variable to the container

cloud_governance/main/test_env.yaml Outdated
@@ -0,0 +1,167 @@
policy: ""
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 should be env.yaml

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The one should copy to env.yaml and pass only defined values.

ebattat
ebattat reviewed Sep 22, 2024
View reviewed changes
Copy link
Collaborator

@ebattat ebattat left a comment

Choose a reason for hiding this comment

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

@athiruma, do you want to update readme documentation with yaml option ?

@athiruma athiruma force-pushed the add_env_yaml branch 2 times, most recently from 59f3a65 to 7525c93 Compare September 23, 2024 08:43
ebattat
ebattat requested changes Sep 23, 2024
View reviewed changes
Copy link
Collaborator

@ebattat ebattat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@athiruma, Can you pls add e2e test that using yaml file ?

@athiruma
Copy link
Collaborator Author

@athiruma, Can you pls add e2e test that using yaml file ?

Added

athiruma
athiruma commented Sep 24, 2024
View reviewed changes
cloud_governance/main/environment_variables.py
Will never override existing attributes.
"""

for yaml_file_path in (os.path.join(os.path.curdir, 'env.yaml'), self.DEFAULT_CONF_PATH):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

searching file in current env.yaml && DEFAULT_CONF_PATH=/tmp/env.yaml

ebattat
ebattat approved these changes Sep 24, 2024
View reviewed changes
Copy link
Collaborator

@ebattat ebattat left a comment

Choose a reason for hiding this comment

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

/LGTM

ebattat
ebattat approved these changes Sep 24, 2024
View reviewed changes
Copy link
Collaborator

@ebattat ebattat left a comment

Choose a reason for hiding this comment

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

/LGTM

@ebattat ebattat merged commit 582f97b into redhat-performance:main Sep 24, 2024
18 checks passed
ebattat
ebattat reviewed Sep 25, 2024
View reviewed changes
.github/workflows/Build.yml
@@ -384,7 +384,15 @@ jobs:
AWS_ACCESS_KEY_ID: ${{ secrets.ACCESS_KEY_ID }}
AWS_SECRET_ACCESS_KEY: ${{ secrets.SECRET_ACCESS_KEY }}
run: |
sudo podman run --rm --name cloud-governance -e policy=${{ matrix.policy }} -e AWS_ACCESS_KEY_ID=$AWS_ACCESS_KEY_ID -e AWS_SECRET_ACCESS_KEY=$AWS_SECRET_ACCESS_KEY -e AWS_DEFAULT_REGION=${{ matrix.region }} -e dry_run=yes -e policy_output=s3://${{ secrets.BUCKET }}/test/${{ matrix.region }} -e log_level=INFO ${{ secrets.QUAY_PUBLIC_CLOUD_GOVERNANCE_REPOSITORY }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@athiruma, I think we should run both yaml and env variabale, WDYT ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah it helps.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pls open new PR and double the e2e test yaml + env
Total should be 104 tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebattat ebattat ebattat approved these changes

Assignees

@athiruma athiruma

Labels
enhancement New feature or request ok-to-test PR ok to test
Projects
Cloud-Governance project
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@athiruma @ebattat