114 use e2 medium for all pro tests in testing env #116
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on modifications to GitHub Actions workflows and configuration files. Key updates include the addition of a conditional job to remove specific strings from a plans file, updates to instance types in testing workflows, enhancements to the Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- .github/workflows/build-release-image.yaml (1 hunks)
- .github/workflows/build-test-image.yaml (20 hunks)
- .gitignore (1 hunks)
- omnistrate.pro.yaml (9 hunks)
Files skipped from review due to trivial changes (1)
- omnistrate.pro.yaml
Additional comments not posted (12)
.gitignore (2)
2-2: Good addition to.gitignore.Adding
.omnistrateto.gitignoreis a good practice to keep the repository clean by not tracking unnecessary files or directories.
3-3: Critical addition to.gitignore.Adding
secretsto.gitignoreis crucial for maintaining the security of the repository by preventing sensitive information from being committed. Great catch!Please ensure that no sensitive information has been accidentally committed before this change. If any sensitive information is found in the commit history, it should be properly removed and revoked.
.github/workflows/build-release-image.yaml (1)
84-87: Verify the impact of removing the- e2-mediumstring.The changes look good! The new step conditionally removes the
- e2-mediumstring from the plans file for the 'pro' environment, enhancing the customization of the release process.Please ensure that removing the
- e2-mediumstring does not have any unintended consequences or break any existing functionality. Consider running tests or manually verifying the behavior of the 'pro' environment after this change..github/workflows/build-test-image.yaml (9)
141-141: LGTM!The instance type change from
e2-custom-4-8192toe2-mediumaligns with the PR objective.
151-151: LGTM!The instance type change from
e2-custom-4-8192toe2-mediumaligns with the PR objective.
161-161: LGTM!The instance type change from
e2-custom-4-8192toe2-mediumaligns with the PR objective.
171-171: LGTM!The instance type change from
e2-custom-4-8192toe2-mediumaligns with the PR objective.
181-181: LGTM!The instance type change from
e2-custom-4-8192toe2-mediumaligns with the PR objective.
191-191: LGTM!The instance type change from
e2-custom-4-8192toe2-mediumaligns with the PR objective.
231-231: LGTM!The instance type change from
e2-custom-4-8192toe2-mediumaligns with the PR objective.
241-241: LGTM!The instance type change from
e2-custom-4-8192toe2-mediumaligns with the PR objective.
251-251: LGTM!The instance type change from
e2-custom-4-8192toe2-mediumaligns with the PR objective.
| @@ -198,7 +198,7 @@ jobs: | |||
| subscriptionId: sub-GJPV3NoNC0 | |||
| serviceId: ${{ vars.OMNISTRATE_INTERNAL_SERVICE_ID }} | |||
| environmentId: ${{ vars.OMNISTRATE_INTERNAL_DEV_ENVIRONMENT}} | |||
| extraParams: "--resource-key 'standalone' --instance-name 'test-standalone-update-memory' --instance-description 'test-standalone-update-memory' --instance-type 'e2-custom-4-8192' --new-instance-type 'e2-custom-8-16384' --storage-size '30' --rdb-config 'medium' --aof-config 'always'" | |||
| extraParams: "--resource-key 'standalone' --instance-name 'test-standalone-update-memory' --instance-description 'test-standalone-update-memory' --instance-type 'e2-medium' --new-instance-type 'e2-custom-4-8192' --storage-size '30' --rdb-config 'medium' --aof-config 'always'" | |||
There was a problem hiding this comment.
Update the new-instance-type parameter to match the PR objective.
The initial instance type has been updated to e2-medium, which aligns with the PR objective. However, the new-instance-type parameter is still set to e2-custom-4-8192.
Consider updating the new-instance-type parameter to a value that is consistent with the PR objective, such as e2-medium or a different instance type that is intended for the memory update test.
| @@ -208,7 +208,7 @@ | |||
| subscriptionId: sub-GJPV3NoNC0 | |||
| serviceId: ${{ vars.OMNISTRATE_INTERNAL_SERVICE_ID }} | |||
| environmentId: ${{ vars.OMNISTRATE_INTERNAL_DEV_ENVIRONMENT}} | |||
| extraParams: "--resource-key 'single-Zone' --instance-name 'test-replication-update-memory' --instance-description 'test-replication-update-memory' --instance-type 'e2-custom-4-8192' --new-instance-type 'e2-custom-8-16384' --storage-size '30' --rdb-config 'medium' --aof-config 'always'" | |||
| extraParams: "--resource-key 'single-Zone' --instance-name 'test-replication-update-memory' --instance-description 'test-replication-update-memory' --instance-type 'e2-medium' --new-instance-type 'e2-custom-4-8192' --storage-size '30' --rdb-config 'medium' --aof-config 'always'" | |||
There was a problem hiding this comment.
Update the new-instance-type parameter to match the PR objective.
The initial instance type has been updated to e2-medium, which aligns with the PR objective. However, the new-instance-type parameter is still set to e2-custom-4-8192.
Consider updating the new-instance-type parameter to a value that is consistent with the PR objective, such as e2-medium or a different instance type that is intended for the memory update test.
| @@ -218,7 +218,7 @@ | |||
| subscriptionId: sub-GJPV3NoNC0 | |||
| serviceId: ${{ vars.OMNISTRATE_INTERNAL_SERVICE_ID }} | |||
| environmentId: ${{ vars.OMNISTRATE_INTERNAL_DEV_ENVIRONMENT}} | |||
| extraParams: "--resource-key 'cluster-Single-Zone' --instance-name 'test-cluster-update-memory' --instance-description 'test-cluster-update-memory' --instance-type 'e2-custom-4-8192' --new-instance-type 'e2-custom-8-16384' --storage-size '30' --rdb-config 'medium' --aof-config 'always' --cluster-replicas '1' --host-count '6'" | |||
| extraParams: "--resource-key 'cluster-Single-Zone' --instance-name 'test-cluster-update-memory' --instance-description 'test-cluster-update-memory' --instance-type 'e2-medium' --new-instance-type 'e2-custom-4-8192' --storage-size '30' --rdb-config 'medium' --aof-config 'always' --cluster-replicas '1' --host-count '6'" | |||
There was a problem hiding this comment.
Update the new-instance-type parameter to match the PR objective.
The initial instance type has been updated to e2-medium, which aligns with the PR objective. However, the new-instance-type parameter is still set to e2-custom-4-8192.
Consider updating the new-instance-type parameter to a value that is consistent with the PR objective, such as e2-medium or a different instance type that is intended for the memory update test.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- falkordb-cluster/cluster-entrypoint.sh (1 hunks)
- falkordb-node/node-entrypoint.sh (1 hunks)
Additional context used
Shellcheck
falkordb-node/node-entrypoint.sh
[warning] 200-200: Quotes/backslashes will be treated literally. Use an array.
(SC2089)
Additional comments not posted (2)
falkordb-cluster/cluster-entrypoint.sh (1)
128-128: LGTM!The addition of the
e2-mediumentry to thememory_limit_instance_type_mapis consistent with the existing structure and enhances the memory configuration options based on the instance type.falkordb-node/node-entrypoint.sh (1)
200-200: Looks good! The change enhances the flexibility of the script.Adding support for the
e2-mediuminstance type expands the utility of theget_memory_limitfunction and allows the script to be used in more diverse environments. This is a positive enhancement.Tools
Shellcheck
[warning] 200-200: Quotes/backslashes will be treated literally. Use an array.
(SC2089)
| @@ -197,7 +197,7 @@ get_self_host_ip() { | |||
|
|
|||
| get_memory_limit() { | |||
|
|
|||
| memory_limit_instance_type_map="{\"e2-custom-small-1024\":\"100MB\",\"e2-custom-4-8192\":\"6GB\",\"e2-custom-8-16384\":\"13GB\",\"e2-custom-16-32768\":\"30GB\",\"e2-custom-32-65536\":\"62GB\"}" | |||
| memory_limit_instance_type_map="{\"e2-custom-small-1024\":\"100MB\",\"e2-medium\":\"2GB\",\"e2-custom-4-8192\":\"6GB\",\"e2-custom-8-16384\":\"13GB\",\"e2-custom-16-32768\":\"30GB\",\"e2-custom-32-65536\":\"62GB\"}" | |||
There was a problem hiding this comment.
Consider refactoring the memory_limit_instance_type_map to use an array.
Shellcheck warns that using a string for the memory_limit_instance_type_map is error-prone because quotes and backslashes will be treated literally. It recommends using an array instead.
Here's a suggested refactoring:
memory_limit_instance_type_map=(
["e2-custom-small-1024"]="100MB"
["e2-medium"]="2GB"
["e2-custom-4-8192"]="6GB"
["e2-custom-8-16384"]="13GB"
["e2-custom-16-32768"]="30GB"
["e2-custom-32-65536"]="62GB"
)
# ...
MEMORY_LIMIT=${memory_limit_instance_type_map[$INSTANCE_TYPE]}This makes the code more robust and maintainable. Let me know if you'd like me to submit a PR with this refactoring.
Tools
Shellcheck
[warning] 200-200: Quotes/backslashes will be treated literally. Use an array.
(SC2089)
dudizimber
deleted the
114-use-e2-medium-for-all-pro-tests-in-testing-env
branch
fix #114
Summary by CodeRabbit
e2-mediuminstance type option in configuration, enhancing resource allocation flexibility..gitignoreto include sensitive files, ensuring better repository security.