Skip to content
This repository was archived by the owner on Nov 1, 2023. It is now read-only.

Trim System.Title if length is > 128#3284

Merged
kananb merged 6 commits into
mainfrom
kanan/trim-system-title
Jul 15, 2023
Merged

Trim System.Title if length is > 128#3284
kananb merged 6 commits into
mainfrom
kanan/trim-system-title

Conversation

@kananb

@kananb kananb commented Jul 11, 2023

Copy link
Copy Markdown
Contributor

Summary of the Pull Request

Trim System.Title when longer than 128 characters. Trims some extra characters to append the first 8 chars of the title hash for collision avoidance.

We probably want to write the full System.Title value somewhere in the bug so that the trimmed information isn't just lost.

What is this about?
#3015

PR Checklist

  • Applies to work item: #xxx
  • CLA signed. If not, go over here and sign the CLI.
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Info on Pull Request

What does this include?

Validation Steps Performed

How does someone test & validate?

@kananb kananb requested review from chkeita, stishkin and tevoinea July 11, 2023 20:55
@codecov-commenter

codecov-commenter commented Jul 11, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #3284 (2c43c2b) into main (930bb3f) will increase coverage by 0.00%.
The diff coverage is 38.09%.

@@           Coverage Diff           @@
##             main    #3284   +/-   ##
=======================================
  Coverage   31.46%   31.46%           
=======================================
  Files         307      307           
  Lines       37202    37219   +17     
=======================================
+ Hits        11704    11712    +8     
- Misses      25498    25507    +9
Impacted Files Coverage Δ
...Service/ApiService/onefuzzlib/notifications/Ado.cs 9.61% <0.00%> (-0.25%) ⬇️
...piService/onefuzzlib/notifications/GithubIssues.cs 0.00% <0.00%> (ø)
src/ApiService/ApiService/OneFuzzTypes/Model.cs 72.70% <100.00%> (+0.03%) ⬆️
...e/onefuzzlib/notifications/JinjaTemplateAdapter.cs 90.81% <100.00%> (+0.06%) ⬆️
...vice/onefuzzlib/notifications/NotificationsBase.cs 70.19% <100.00%> (+1.19%) ⬆️

@kananb kananb force-pushed the kanan/trim-system-title branch from 54f43c8 to ff98a92 Compare July 11, 2023 21:29
Porges
Porges reviewed Jul 11, 2023
View reviewed changes
Comment thread src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs Outdated
@Porges

Porges commented Jul 11, 2023

Copy link
Copy Markdown
Member

Agree about this:

We probably want to write the full System.Title value somewhere in the bug so that the trimmed information isn't just lost.

Could we add an additional field to the template for the full title and always include it?

@kananb

kananb commented Jul 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Could we add an additional field to the template for the full title and always include it?

We can probably put it at the top of the bug's "Repro Steps". I don't think we have much control over the fields since they're org specific.

@kananb kananb force-pushed the kanan/trim-system-title branch 2 times, most recently from ae1017f to a9e9dc2 Compare July 11, 2023 23:03
@kananb kananb force-pushed the kanan/trim-system-title branch from a9e9dc2 to 7c053dc Compare July 12, 2023 15:45
@Porges

Porges commented Jul 12, 2023

Copy link
Copy Markdown
Member

Could we add an additional field to the template for the full title and always include it?

We can probably put it at the top of the bug's "Repro Steps". I don't think we have much control over the fields since they're org specific.

Sorry, I was imprecise here, I meant a variable for the Scriban template to render, not the ADO fields.

@mgreisen

mgreisen commented Jul 12, 2023

Copy link
Copy Markdown
Contributor

@kananb how did we decide on the 128 char limit?

@kananb kananb force-pushed the kanan/trim-system-title branch from 40e9989 to 77debf0 Compare July 14, 2023 00:00
@kananb

kananb commented Jul 14, 2023

Copy link
Copy Markdown
Contributor Author

@kananb how did we decide on the 128 char limit?

https://github.com/MicrosoftDocs/azure-devops-docs/issues/5890#issuecomment-539632059

@kananb kananb force-pushed the kanan/trim-system-title branch from 77debf0 to 2c43c2b Compare July 15, 2023 01:25
@kananb kananb enabled auto-merge (squash) July 15, 2023 01:26
@kananb kananb merged commit f958cb7 into main Jul 15, 2023
@kananb kananb deleted the kanan/trim-system-title branch July 15, 2023 01:46
AdamL-Microsoft added a commit that referenced this pull request Jul 17, 2023
@AdamL-Microsoft
Adding PRs #3284 #3279
e30bbb7
@AdamL-Microsoft AdamL-Microsoft mentioned this pull request Jul 17, 2023
Merged
@kananb kananb mentioned this pull request Jul 17, 2023
Closed
tevoinea
tevoinea reviewed Jul 24, 2023
View reviewed changes
Comment thread src/ApiService/ApiService/onefuzzlib/notifications/NotificationsBase.cs
return new Renderer(
container,
filename,
issueTitle,

@tevoinea tevoinea Jul 24, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's render the title, untrimmed.

@kananb kananb mentioned this pull request Jul 24, 2023
Merged
5 tasks
@tevoinea tevoinea mentioned this pull request Aug 1, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stishkin stishkin Awaiting requested review from stishkin
3 more reviewers
@tevoinea tevoinea tevoinea left review comments
@Porges Porges Porges approved these changes
@chkeita chkeita chkeita approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@kananb @codecov-commenter @Porges @mgreisen @chkeita @tevoinea