fix: reduce number of roles required to run in cluster #195
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- removes operator.openshift.io/v1 Console - remove route.openshift.io Route and use static internal OCP service - load client config from in-cluster and kubeconfig - get bearer token via client api - mandatory BASE_DOMAIN env variable Signed-off-by: Tomas Turek <[email protected]>
Reviewer's GuideThis PR refactors script.py to eliminate direct use of OpenShift Console and Route APIs by loading configuration more flexibly, retrieving the bearer token via the client API, and using static/internal services accessed over HTTP; it also enforces BASE_DOMAIN as a required environment variable and streamlines the nightly execution flow. Class diagram for updated authentication and configuration loadingclassDiagram
class Script {
+openshift_setup()
+get_bearer_token()
}
class client {
+ApiClient
+Configuration
}
Script ..> client : uses
Script : +openshift_setup() loads config from in-cluster or kubeconfig
Script : +get_bearer_token() retrieves token from client.ApiClient
Flow diagram for new Thanos Querier status checkflowchart TD
A[Start] --> B[Get THANOS_QUERIER_URL from env or default]
B --> C[Get bearer token via client API]
C --> D[Check Thanos Querier API status]
D -->|Success| E[Proceed with nightly metrics]
D -->|Failure| F[Fail job]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @osmman - I've reviewed your changes - here's some feedback:
Blocking issues:
- time.sleep() call; did you mean to leave this in? (link)
General comments:
- Standardize return types and error handling in the status-check functions so callers can unambiguously distinguish success, failure, and exceptions (e.g., avoid mixing bools and ints and catch specific exception types rather than using bare except).
- Switch from print statements to the logging module to provide configurable log levels and better observability in cluster environments.
- Ensure service URLs always include the protocol (https://) when formatting the query endpoints to prevent intermittent connection failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Standardize return types and error handling in the status-check functions so callers can unambiguously distinguish success, failure, and exceptions (e.g., avoid mixing bools and ints and catch specific exception types rather than using bare except).
- Switch from print statements to the logging module to provide configurable log levels and better observability in cluster environments.
- Ensure service URLs always include the protocol (https://) when formatting the query endpoints to prevent intermittent connection failures.
## Individual Comments
### Comment 1
<location> `src/script.py:60` </location>
<code_context>
- route_up = False
- thanos_quierier_host = ''
+ headers = {'Authorization': '{bearer_token}'.format(bearer_token=bearer_token)}
while attempt < attempts:
try:
</code_context>
<issue_to_address>
Authorization header may be missing 'Bearer' prefix.
Omitting the 'Bearer' prefix may cause authentication failures. Update the header to include 'Bearer' if required by the API: {'Authorization': 'Bearer {bearer_token}'}.
</issue_to_address>
### Comment 2
<location> `src/script.py:126` </location>
<code_context>
+ fulcio_new_certs_query_URL = '{thanos_quierier_host}/api/v1/query?&{fulcio_new_certs_query_data}'.format(thanos_quierier_host=thanos_quierier_host, fulcio_new_certs_query_data=fulcio_new_certs_query_data)
</code_context>
<issue_to_address>
Protocol removed from query URLs may cause issues if host is not a full URL.
The new code relies on thanos_quierier_host including the protocol. Please validate or document this requirement to prevent request failures.
</issue_to_address>
### Comment 3
<location> `src/script.py:102` </location>
<code_context>
- return 1
+ bearer_token = configuration.api_key.get('authorization')
+
+ if not bearer_token:
+ raise RuntimeError("Bearer token not found in the loaded configuration.")
+
</code_context>
<issue_to_address>
Raising RuntimeError may not be handled upstream.
If abrupt termination is not desired, consider returning a sentinel value or handling the error more gracefully.
</issue_to_address>
## Security Issues
### Issue 1
<location> `src/script.py:74` </location>
<issue_to_address>
**security (python.lang.best-practice.arbitrary-sleep):** time.sleep() call; did you mean to leave this in?
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| headers = {'Authorization': '{bearer_token}'.format(bearer_token=bearer_token)} |
There was a problem hiding this comment.
issue (bug_risk): Protocol removed from query URLs may cause issues if host is not a full URL.
The new code relies on thanos_quierier_host including the protocol. Please validate or document this requirement to prevent request failures.
| from nightly import main_nightly | ||
| from installation import main_installation | ||
|
|
||
| def openshift_setup(): | ||
| config.load_incluster_config() | ||
| try: | ||
| config.load_incluster_config() |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Inline variable that is immediately returned (
inline-immediately-returned-variable) - Explicitly raise from a previous error (
raise-from-previous-error)
Comment on lines
+63
to
73
| response = fetch_response_data(query_url+"/api/v1/status/buildinfo", headers, REQUESTS_CA_BUNDLE, REQUESTS_CA_BUNDLE_INTERNAL) | ||
| print(response) | ||
| if response.status_code == 200 or response.status_code == 201: | ||
| return True | ||
| else: | ||
| print('API is not accessible yet. Retrying in ', sleep_interval, ' seconds...') | ||
| attempt = attempt + 1 | ||
| time.sleep(sleep_interval) | ||
| except requests.exceptions.RequestException as e: | ||
| print(f"Request failed with error: {e}") | ||
| attempt = attempt + 1 |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use f-string instead of string concatenation (
use-fstring-for-concatenation) - Replace multiple comparisons of same variable with
inoperator (merge-comparisons) - Replace assignment with augmented assignment [×2] (
aug-assign)
Comment on lines
+100
to
+103
| bearer_token = configuration.api_key.get('authorization') | ||
|
|
||
| if not bearer_token: | ||
| raise RuntimeError("Bearer token not found in the loaded configuration.") |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Lift code into else after jump in control flow (
reintroduce-else) - Swap if/else branches (
swap-if-else-branches)
| rekor_qps_by_api_query_URL='https://{thanos_quierier_host}/api/v1/query?&{rekor_qps_by_api_query_data}'.format(thanos_quierier_host=thanos_quierier_host, rekor_qps_by_api_query_data=rekor_qps_by_api_query_data) | ||
| headers = {'Authorization': 'Bearer {bearer_token}'.format(bearer_token=bearer_token)} | ||
| rekor_qps_by_api_query_URL='{thanos_quierier_host}/api/v1/query?&{rekor_qps_by_api_query_data}'.format(thanos_quierier_host=thanos_quierier_host, rekor_qps_by_api_query_data=rekor_qps_by_api_query_data) | ||
| headers = {'Authorization': '{bearer_token}'.format(bearer_token=bearer_token)} | ||
|
|
||
| fulcio_new_certs_response_data = fetch_response_data(fulcio_new_certs_query_URL, headers, REQUESTS_CA_BUNDLE, REQUESTS_CA_BUNDLE_INTERNAL) | ||
| if fulcio_new_certs_response_data.status_code == 200 or fulcio_new_certs_response_data.status_code == 201: |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Replace multiple comparisons of same variable with
inoperator (merge-comparisons) - Low code quality found in query_nightly_metrics - 17% (
low-code-quality)
Suggested change
| if fulcio_new_certs_response_data.status_code == 200 or fulcio_new_certs_response_data.status_code == 201: | |
| if fulcio_new_certs_response_data.status_code in [200, 201]: |
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
| REQUESTS_CA_BUNDLE_INTERNAL = os.environ.get('REQUESTS_CA_BUNDLE_INTERNAL') | ||
| REQUESTS_CA_BUNDLE = os.environ.get('REQUESTS_CA_BUNDLE') | ||
| query_nightly_metrics(openshift_client, thanos_quierier_host, bearer_token, base_domain, REQUESTS_CA_BUNDLE, REQUESTS_CA_BUNDLE_INTERNAL) | ||
| requests_ca_bundle_internal = os.environ.get('REQUESTS_CA_BUNDLE_INTERNAL') |
There was a problem hiding this comment.
issue (code-quality): Extract code out into function (extract-method)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Reduce required OpenShift cluster roles by removing operator and route API dependencies, consolidate config loading and token retrieval, mandate BASE_DOMAIN, and switch to HTTP health checks for Thanos Querier.
Enhancements: