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

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

Merged
merged 3 commits into from
Feb 13, 2024
Merged

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0876385
Add an option for adding a separate display name for repositories.
emjin Feb 9, 2024
0b2e8ca
Make repo_display_name optional
emjin Feb 12, 2024
a673998
Merge main
emjin Feb 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions semgrep_output_v1.atd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1110,6 +1110,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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@emjin emjin Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(and typed way)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

This allows monorepos to be scanned as separate projects. *)
repo_display_name;

(* TODO: the branch should use a standard format? like refs/... ? or it can
* be a basic branch name like 'foobar'?
Expand Down
Loading