Skip to content

Optimizations can get data without HTTP #1090

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

Merged
merged 4 commits into from
Mar 8, 2025
Merged

Conversation

ArneTR
Copy link
Member

@ArneTR ArneTR commented Mar 8, 2025

This PR introduces a fallback method that we plan to make the default for getting optimizations data.

It does not use the old HTTP transport mechanism, but queries the database directly.

Pros:

  • It can handle authentication seprately and thus allow for users in shared environments, which however have restricted view configurations for the API

Downs:

  • It bypasses Redis caching. Howver at the point in time where the optimizations run no Redis cache has been built anyway. However we loose the chance to write to the cache here.

Optimizations can still happen asynchronously if wanted on a different box

Greptile Summary

This PR modifies container name resolution to support direct database access for optimization data retrieval in shared environments, focusing on Redis container configuration and host file updates.

  • Added Redis container resolution (green-coding-redis-container) to localhost entry in /etc/hosts via lib/install_shared.sh
  • Modified Redis port exposure to 127.0.0.1:6379 in docker/compose.yml.example for network isolation
  • Updated tests/edit-etc-hosts.sh to include Redis container hostname for testing
  • Ensures proper container resolution while maintaining security in shared environments

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

4 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

#pylint: disable=dangerous-default-value
def run_reporters(run_id, repo_path, optimizations_ignore=[]):
def run_reporters(user_id, run_id, repo_path, optimizations_ignore=[]):
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Mutable default argument optimizations_ignore=[] can cause unexpected behavior if modified between calls

Suggested change
def run_reporters(user_id, run_id, repo_path, optimizations_ignore=[]):
def run_reporters(user_id, run_id, repo_path, optimizations_ignore=None):

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

LGTM

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

LGTM

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@ArneTR ArneTR merged commit 5504a9a into main Mar 8, 2025
1 check failed
@ArneTR ArneTR deleted the optimizations-for-shared-envs branch March 8, 2025 19:22
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

LGTM

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@@ -153,7 +153,7 @@ function prepare_config() {

if [[ $modify_hosts == true ]] ; then

local etc_hosts_line_1="127.0.0.1 green-coding-postgres-container"
local etc_hosts_line_1="127.0.0.1 green-coding-postgres-container green-coding-redis-container"
Copy link
Member

Choose a reason for hiding this comment

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

This will create 2 entries for 127.0.0.1 green-coding-postgres-container in the /etc/hosts file as the grep command will not find the line and then add it again (with the Redis part). Nothing serious

Copy link
Member

Choose a reason for hiding this comment

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

@ArneTR 👆

ArneTR added a commit that referenced this pull request Mar 14, 2025
* main: (54 commits)
  Optimizations can get data without HTTP (#1090)
  Checking int fields for params and limiting retrieval amount (#1089)
  E-Mail now made optional
  Bump deepdiff from 8.2.0 to 8.3.0 (#1087)
  Bump psycopg-pool from 3.2.5 to 3.2.6 (#1081)
  Bump pytest from 8.3.4 to 8.3.5 (#1084)
  Bump fastapi[standard] from 0.115.8 to 0.115.11 (#1083)
  Update README.md - projects -> products
  Update README.md
  Updated gcb_playwright to 1.50.0 Will be build with 1.50.1 PR
  Bump psycopg-pool from 3.2.4 to 3.2.5 (#1078)
  Bump playwright/python in /docker/auxiliary-containers/gcb_playwright (#1072)
  Bump aiohttp from 3.11.12 to 3.11.13 (#1080)
  Bump psycopg[binary] from 3.2.4 to 3.2.5 (#1077)
  Bump cachetools from 5.5.1 to 5.5.2 (#1076)
  Updated Cloud Energy
  Typo
  Eco-CI now has a carbon badge in the repo overview [skip ci] (#1079)
  Updated Cloud Energy
  No asking for send ping in Codespaces; When asked though never send ping automatically
  ...
davidkopp pushed a commit to davidkopp/green-metrics-tool that referenced this pull request Mar 14, 2025
* Optimizations can get data without HTTP

* Fix for immutable default argument [skip ci]

* Redis now binds to local port to allow TCP connections - Used only in testing

* Adding container to /etc/hosts
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.

2 participants