Add an option for adding a separate display name for repositories. #223

emjin · 2024-02-09T18:13:18Z

I ran make setup && make to update the generated code after editing a .atd file (TODO: have a CI check)
I made sure we're still backward compatible with old versions of the CLI.
For example, the Semgrep backend need to still be able to consume data generated
by Semgrep 1.17.0.
See https://atd.readthedocs.io/en/latest/atdgen-tutorial.html#smooth-protocol-upgrades

github-actions · 2024-02-09T18:14:33Z

Backwards compatibility summary:

Checking backward compatibility of semgrep_output_v1.atd against past version v1.29.0
Skipping v1.30.0 because commit 78720c795cd5a186f5102c87125ef876c6435a0c has already been checked
Skipping v1.31.0 because commit 78720c795cd5a186f5102c87125ef876c6435a0c has already been checked
Skipping v1.31.1 because commit 78720c795cd5a186f5102c87125ef876c6435a0c has already been checked
Skipping v1.31.2 because commit 78720c795cd5a186f5102c87125ef876c6435a0c has already been checked
Skipping v1.32.0 because commit 78720c795cd5a186f5102c87125ef876c6435a0c has already been checked
Checking backward compatibility of semgrep_output_v1.atd against past version v1.33.0
Skipping v1.33.1 because commit 8849e56ddb0977e38a120a6cfbd1c396eb6fa15e has already been checked
Skipping v1.33.2 because commit 8849e56ddb0977e38a120a6cfbd1c396eb6fa15e has already been checked
Checking backward compatibility of semgrep_output_v1.atd against past version v1.34.0
Checking backward compatibility of semgrep_output_v1.atd against past version v1.37.0
Checking backward compatibility of semgrep_output_v1.atd against past version v1.38.0
Skipping v1.38.1 because commit fd294683e7369cabf63738febeaba8a22c925187 has already been checked
Skipping v1.38.2 because commit fd294683e7369cabf63738febeaba8a22c925187 has already been checked
Skipping v1.38.3 because commit fd294683e7369cabf63738febeaba8a22c925187 has already been checked
Checking backward compatibility of semgrep_output_v1.atd against past version v1.39.0
Checking backward compatibility of semgrep_output_v1.atd against past version v1.40.0
Checking backward compatibility of semgrep_output_v1.atd against past version v1.41.0
Checking backward compatibility of semgrep_output_v1.atd against past version v1.42.0
Checking backward compatibility of semgrep_output_v1.atd against past version v1.43.0
Checking backward compatibility of semgrep_output_v1.atd against past version v1.44.0
Checking backward compatibility of semgrep_output_v1.atd against past version v1.45.0
Checking backward compatibility of semgrep_output_v1.atd against past version v1.46.0
Checking backward compatibility of semgrep_output_v1.atd against past version v1.47.0
Skipping v1.48.0 because commit 278ed753e0c66b8bfc3f2d805fde53be022dd4b6 has already been checked
Checking backward compatibility of semgrep_output_v1.atd against past version v1.49.0
Skipping v1.50.0 because commit 857682f41eb09e0b330a247ff1adf3bfeaf9d9ca has already been checked
Checking backward compatibility of semgrep_output_v1.atd against past version v1.52.0
Checking backward compatibility of semgrep_output_v1.atd against past version v1.53.0
Skipping v1.54.0 because commit 3b72d494260258497e796d094b1a4916501a6df1 has already been checked
Skipping v1.54.1 because commit 3b72d494260258497e796d094b1a4916501a6df1 has already been checked
Checking backward compatibility of semgrep_output_v1.atd against past version v1.54.2
Skipping v1.54.3 because commit 9f1c50383a9a9969e2fe7a5f9bff9ca0a7c837bb has already been checked
Checking backward compatibility of semgrep_output_v1.atd against past version v1.55.0
Skipping v1.55.1 because commit 6dffeaa692153fd33b4f154fddaefde1f2f1ae27 has already been checked
Skipping v1.55.2 because commit 6dffeaa692153fd33b4f154fddaefde1f2f1ae27 has already been checked
Checking backward compatibility of semgrep_output_v1.atd against past version v1.56.0
Checking backward compatibility of semgrep_output_v1.atd against past version v1.57.0
Skipping v1.58.0 because commit 4cc11b00d411c02fc611aa8c78a336520438fb48 has already been checked
Checking backward compatibility of semgrep_output_v1.atd against past version v1.59.0
Checking backward compatibility of semgrep_output_v1.atd against past version v1.59.1
Checking backward compatibility of semgrep_output_v1.atd against past version v1.60.0
Skipping v1.60.1 because commit eed58a091fd7d19e402a6d4cf2d287e137215d03 has already been checked

semgrep_output_v1.atd

aryx · 2024-02-13T10:58:04Z

semgrep_output_v1.atd

@@ -1111,6 +1111,10 @@ type project_metadata = {
    ?repo_id: string option;
    (* a.k.a repository owner id *)
    ?org_id: string option;
+
+    (* Users can set a different name for display and for PR comments.


Note that I introduced a ci_config_from_cloud type in this file, that has a dirs_config: field
especially designed for monorepos, to allow to have different configuration per directory.
It's not used yet by the backend, but I think we should start using it, to become monorepo friendly.

@chmccreery I know your team has other priorities but I am pretty concerned that the env var method of configuring monorepos won't be ergonomic, which would still hamper adoption. Would love to sync on what we could or couldn't accomplish quickly with a cross-team project on monorepos.

Yes, I think we should go towards the ci_config_from_cloud data structure I defined above.
That way we get all the info from the cloud in a uniform way.

(and typed way)

Yeah, I am in no way wedded to env variables, though I think they are useful for allowing customization/overrides. If you wanted to auto-detect this variable for monorepos, that seems fine as well to me! Maybe I'm missing the context on what would be needed from the backend in this case.

But I agree we need to support multiple versions of the CLI, which is why martin actually implemented this tool, atddiff, that statically check breaking compatibility changes.

the CI would just run 'semgrep ci'

I'm not sure this is what users want. I'm not sure what they want (I haven't spoken to people about this) but I imagine that if your entire codebase is one monorepo you could want to let your teams run semgrep separately on each repo, the way we do for semgrep vs semgrep-app. Maybe worth speaking to Milan first to figure out the preferred use.

The other concern with one semgrep ci run is that I'm worried it doesn't actually help us solve the main problem, which is CI timing out on long semgrep scans.

This random github thread is getting long; @chmccreery do you have a preferred place to discuss something like this? Maybe an "RFC" ticket on Findings?

maybe ... milan knows maybe better the actual use case. But anyway, I'd rather not (ab)use environment variables to store useful information; it's not typed, it's not explicit, it's fragile. I'd rather have actual fields in semgrep_output_v1.atd

Add an option for adding a separate display name for repositories.

0876385

emjin marked this pull request as draft February 9, 2024 18:13

aryx requested changes Feb 11, 2024

View reviewed changes

semgrep_output_v1.atd Outdated Show resolved Hide resolved

emjin added 2 commits February 12, 2024 15:03

Make repo_display_name optional

0b2e8ca

Merge main

a673998

emjin requested a review from aryx February 13, 2024 00:35

emjin marked this pull request as ready for review February 13, 2024 00:38

aryx approved these changes Feb 13, 2024

View reviewed changes

emjin merged commit bbfd1c5 into main Feb 13, 2024
3 checks passed

emjin deleted the emma/gh-8953-allow-overriding-display-name branch February 13, 2024 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an option for adding a separate display name for repositories. #223

Add an option for adding a separate display name for repositories. #223

emjin commented Feb 9, 2024 •

edited

Loading

github-actions bot commented Feb 9, 2024 •

edited

Loading

aryx Feb 13, 2024

emjin Feb 13, 2024 •

edited

Loading

aryx Feb 13, 2024

aryx Feb 13, 2024

chmccreery Feb 15, 2024

aryx Feb 16, 2024

emjin Feb 16, 2024

emjin Feb 16, 2024

emjin Feb 16, 2024

aryx Feb 16, 2024

Add an option for adding a separate display name for repositories. #223

Add an option for adding a separate display name for repositories. #223

Conversation

emjin commented Feb 9, 2024 • edited Loading

github-actions bot commented Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

emjin Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emjin commented Feb 9, 2024 •

edited

Loading

github-actions bot commented Feb 9, 2024 •

edited

Loading

emjin Feb 13, 2024 •

edited

Loading