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

Add check to require new specs use TypeSpec #27227

Merged
merged 15 commits into from
Jan 4, 2024
Merged

Add check to require new specs use TypeSpec #27227

merged 15 commits into from
Jan 4, 2024

Conversation

mikeharder
Copy link
Member

@mikeharder mikeharder commented Jan 3, 2024 •
edited
Loading

@openapi-pipeline-app openapi-pipeline-app
Copy link

openapi-pipeline-app bot commented Jan 3, 2024 •
edited
Loading

Next Steps to Merge

✔️ All automated merging requirements have been met! Refer to step 4 in the PR workflow diagram (even if your PR is for data plane, not ARM).

@openapi-pipeline-app openapi-pipeline-app
Copy link

openapi-pipeline-app bot commented Jan 3, 2024 •
edited
Loading

Swagger Validation Report

️️✔️BreakingChange succeeded [Detail] [Expand] 
There are no breaking changes.
️️✔️Breaking Change(Cross-Version) succeeded [Detail] [Expand] 
There are no breaking changes.
️️✔️CredScan succeeded [Detail] [Expand] 
There is no credential detected.
️️✔️LintDiff succeeded [Detail] [Expand] 
Validation passes for LintDiff.
️️✔️Avocado succeeded [Detail] [Expand] 
Validation passes for Avocado.
️️✔️SwaggerAPIView succeeded [Detail] [Expand]
️️✔️TypeSpecAPIView succeeded [Detail] [Expand]
️️✔️ModelValidation succeeded [Detail] [Expand] 
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand] 
Validation passes for SemanticValidation.
️️✔️PoliCheck succeeded [Detail] [Expand] 
Validation passed for PoliCheck.
️️✔️SpellCheck succeeded [Detail] [Expand] 
Validation passes for SpellCheck.
️️✔️Lint(RPaaS) succeeded [Detail] [Expand] 
Validation passes for Lint(RPaaS).
️️✔️PR Summary succeeded [Detail] [Expand] 
Validation passes for Summary.
️️✔️Automated merging requirements met succeeded [Detail] [Expand]
Posted by Swagger Pipeline | How to fix these errors?

@openapi-pipeline-app openapi-pipeline-app
Copy link

openapi-pipeline-app bot commented Jan 3, 2024 •
edited
Loading

Swagger Generation Artifacts

️️✔️ApiDocPreview succeeded [Detail] [Expand]
️️✔️SDK Breaking Change Tracking succeeded [Detail] [Expand]

Breaking Changes Tracking

Posted by Swagger Pipeline | How to fix these errors?

@openapi-pipeline-app openapi-pipeline-app
Copy link

openapi-pipeline-app bot commented Jan 3, 2024 •
edited
Loading

PR validation pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment.

@mikeharder mikeharder self-assigned this Jan 4, 2024
@mikeharder mikeharder marked this pull request as ready for review January 4, 2024 06:51
@mikeharder mikeharder requested a review from kurtzeborn January 4, 2024 06:52
@konrad-jamrozik
Copy link

konrad-jamrozik commented Jan 4, 2024 •
edited
Loading

I noticed this renders like that:

image

Note the similar names, but one with underscore.

Is this intended? Just double-checking.

Also: do we maybe want to differentiate the name of the job vs the name of the task more?

@konrad-jamrozik
Copy link

konrad-jamrozik commented Jan 4, 2024 •
edited
Loading

I see such log in the example provided:

image

How about we ensure that the log outputs clickable URL to the PR? This way it will be a little bit convenient to navigate from the failed ADO task to the PR.

weshaggard
weshaggard reviewed Jan 4, 2024
View reviewed changes
eng/scripts/TypeSpec-Requirement.ps1
}

# Example: specification/foo/resource-manager/Microsoft.Foo
$pathToServiceName = ($file -split '/')[0..3] -join '/'
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth later using the regex above to get the path so we aren't blindly taking path segments that we don't know are what we expect.

weshaggard
weshaggard reviewed Jan 4, 2024
View reviewed changes
Copy link
Member

@weshaggard weshaggard left a comment

Choose a reason for hiding this comment

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

Left a few comments but otherwise we can get this in to start using and tweak it from there.

@mikeharder mikeharder merged commit 3a21604 into main Jan 4, 2024
34 checks passed
@mikeharder mikeharder deleted the mikeharder/require-typespec branch January 4, 2024 22:01
@mikeharder
Copy link
Member Author

I noticed this renders like that:

image

Note the similar names, but one with underscore.

Is this intended? Just double-checking.

Also: do we maybe want to differentiate the name of the job vs the name of the task more?

Job names cannot contain spaces, but step names can, hence my naming scheme. The default job name is just "Job" which looks awful to me, so I prefer to replace with something. Happy to rename if you have a better idea.

@mikeharder
Copy link
Member Author

I see such log in the example provided:

image

How about we ensure that the log outputs clickable URL to the PR? This way it will be a little bit convenient to navigate from the failed ADO task to the PR.

Do we do this in other checks? If so, I'm happy to add to this check, otherwise I'd rather not for consistency.

@konrad-jamrozik
Copy link

konrad-jamrozik commented Jan 5, 2024 •
edited
Loading

@mikeharder

Re TypeSpec_Requirement and TypeSpec Requirement: I was just a low-key concerned about a conversation like that:

  • Open the "TypeSpec Requirement" logs
  • Which one is it? I see two.
  • The one with space, not underscore

Which is a bit silly. But it is a remote concern. Still better than having two exactly the same names, which we often have in openai-alps 🤦.

Maybe we could call the task/step e.g. Check TypeSpec presence or Verify TypeSpec requirement or something. But not a big deal. Can stay the way it is rn, no probs.

Re URL in logs: I don't think our checks diag. logs try to stay consistent with other checks in any way, so I wouldn't be worried about that. But this is a minor convenience anyway.

@weshaggard
Copy link
Member

Job names cannot contain spaces, but step names can, hence my naming scheme. The default job name is just "Job" which looks awful to me, so I prefer to replace with something. Happy to rename if you have a better idea.

You can get around the space issue by using displayName, displayName: TypeSpec Requirement

arifibrahim4 pushed a commit that referenced this pull request Feb 26, 2024
@mikeharder @arifibrahim4
[TypeSpec Requirement] Add check to require new specs use TypeSpec (#…
5553c4e 
…27227)

- Fixes #25138
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@konrad-jamrozik konrad-jamrozik konrad-jamrozik approved these changes

@weshaggard weshaggard weshaggard approved these changes

@kurtzeborn kurtzeborn Awaiting requested review from kurtzeborn

Assignees

@mikeharder mikeharder

Labels
None yet
Projects
Azure SDK EngSys 🤖🧠
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TypeSpec Validation] Add rule to require all new specs use TypeSpec
3 participants
@mikeharder @konrad-jamrozik @weshaggard