search

Azure / azure-sdk-tools

Tools repository leveraged by the Azure SDK team.
MIT License
111 stars 176 forks source link

Remove obsolete `Swagger Generation Artifacts` comment #8612

Closed konrad-jamrozik closed 2 months ago

konrad-jamrozik commented 3 months ago

These comments (example):

image

Reasons:

@raych1 @weshaggard FYI

raych1 commented 3 months ago

One impact is that the history pipeline run link is missing if removing the PR comment, leaving users without a way to access it. image

In addition, users will need to be guided to browse to Checks tab to view the check result.

+@lirenhe @ArthurMa1978 @weidongxu-microsoft to comment on the concerns.

konrad-jamrozik commented 3 months ago

@raych1 we are moving towards the model of guiding users to the checks tab, and ensuring the PR has only one comment that users must review: the Next Steps to Merge. Experience has shown that users get overwhelmed by too many comments on the PR and fail to notice the Next Steps to Merge comment, causing support burden for us (very often we tell the users to just go and read that comment).

The checks tab does have history in it for each check. In addition, the Next Steps to Merge comment has a history itself that will include information which mandatory SDK checks have failed and when. Hence both concerns should be alleviated by this.

Additional context in this document

raych1 commented 2 months ago

@konrad-jamrozik can you share me where I can view the history run of the CI checks? I remember it needs to query the pipeline run by the PR# on the page of https://dev.azure.com/azure-sdk/internal/_build?definitionId=1736&_a=summary.

konrad-jamrozik commented 2 months ago

@raych1 yeah, so few options:

1. Query the ADO pipeline runs, as you mentioned

2. Use the GitHub UI for check

Note that if you open view of a previous checkRun, the View Azure DevOps build log for more details link will point to corresponding, previous, ADO run.

3. Query our Kusto cluster for the logs

See this function I wrote.

konrad-jamrozik commented 2 months ago

Pull Request 565678: Remove "Swagger Generation Artifacts" comment

raych1 commented 2 months ago

1. Query the ADO pipeline runs, as you mentioned

2. Use the GitHub UI for check

3. Query our Kusto cluster for the logs

The second approach only associates with the latest run for a specific commit. For multiple pipeline runs for a given commit, it relies on the first approach. Therefore, while it technically allows for history queries, it requires additional effort.

konrad-jamrozik commented 2 months ago

@raych1 yes, it is in line with our 20/80 approach - 20% effort (i.e. using just the checks tab instead of coding entire new comment) to get the 80% result (i.e. the few extra steps people have to take by going through checks tab).

The general idea is that the ROI is too low to try to polish it to, and pay the maintenance cost, of a 100% solution. There are other more urgent matters to attend to that are currently at 0%.