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

Jit - Secret Detection Test #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Jit - Secret Detection Test #1

wants to merge 1 commit into from

Conversation

jit-ci[bot]
Copy link

@jit-ci jit-ci bot commented Jun 5, 2023

No description provided.

step-security-bot
step-security-bot reviewed Jun 5, 2023
View reviewed changes
Copy link

@step-security-bot step-security-bot left a comment

Choose a reason for hiding this comment

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

Please find StepSecurity AI-CodeWise code comments below.

Code Comments

jit_secret_test_tile.py

  • [High]Avoid using all uppercase letters for variable names
    According to PEP8, variable names should be in lowercase with words separated by underscores. my_variable = "Hello World"
  • [Medium]Initialize variables at the point of declaration
    Initializing variables at the beginning of a block of code can reduce readability and increase the likelihood of introducing bugs. my_variable = "Hello World"
  • [Low]Use double quote marks for strings instead of single quote marks
    According to PEP8, double quotes should be preferred for string literals. my_variable = "Hello World"

Feedback

We appreciate your feedback in helping us improve the service! To provide feedback, please use emojis on this comment. If you find the comments helpful, give them a 👍. If they aren't useful, kindly express that with a 👎. If you have questions or detailed feedback, please create n GitHub issue in StepSecurity/AI-CodeWise.

nipundev pushed a commit that referenced this pull request Nov 30, 2023
@bobheadxi
telemetry: support float64 attributes (sourcegraph#58643)
eca0acc 
In Javascript, there is no way to easily determine if a number is an integer or a float, as JS numbers are [floats](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number). This makes `value: Int!` hard to use, because there is no way to ask callers to provide metadata in integer values only. To support clients (typically in JS/TS) well, we need to accept `value: Float!` - however, this is a breaking change:

```
In element #0: In field "parameters": In field "metadata": In element #1: In field "value": Expected type "Float", found 0. (line 3, column 28)
```

To make this change smoothly, we accept `value: JSONValue` in the GraphQL input and validate that it is a numeric value in the backend - this allows clients to provide both integers and floats.

For events export, the change is simpler - we add a new field `metadata = 5`, and mark the existing metadata field as deprecated (`legacy_metadata`). In proto, only the field index matters, so the rename is safe, and ensures there is no disruption for data going to BigQuery, since we can continue to use `metadata` as the JSON field and simply drop `legacy_metadata` entirely.

For existing clients, out-of-band telemetry clients (namely Cody) will need to be updated to round values to integers unless interacting with a more recent Sourcegraph version, e.g. sourcegraph/cody#1913 (comment)

## Test plan

- Unit tests validating GraphQL schema works as expected, including back-compat
- Unit tests on telemetry gateway field migration
- Eixsting tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@step-security-bot step-security-bot step-security-bot left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@step-security-bot