Skip to content

Karenchen/enable draftv2#4651

Merged
zhoxing-ms merged 78 commits intoAzure:mainfrom
karenychen:karenchen/enable-draftv2
May 23, 2022
Merged

Karenchen/enable draftv2#4651
zhoxing-ms merged 78 commits intoAzure:mainfrom
karenychen:karenchen/enable-draftv2

Conversation

@karenychen
Copy link
Copy Markdown
Contributor

@karenychen karenychen commented Apr 7, 2022 •
edited
Loading

This is an extension for DraftV2. We have created a new feature branch as Marcus is away for the rest of the week
DraftV2 Repo
DraftV2 Design Dock

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

mahines and others added 26 commits March 21, 2022 10:57
starting point
547441e
command makes call correctly. Need to reach out to DraftV2
4aacf22
attempting to download binaries, but headers may be blocking
6cd1e00
Downloading binary, but it's not located in the correct path
48ec4bc
download into the correct directory works. Need to check existence of…
a15212e 
… binary
adding check to ensure download is correct for users os
1eceebf
download and run binary
6f775df
fixing msg
577a65a
adding copywrite comment
cac9fc8
addressng PR comments, function naming conventions, etc.
275ea32
fixing copywrite typo
29db02f
The linter seems to be complaining about the List type hints. Resolvi…
8b05aa4 
…ng other issues with whitespace, and unused variables
fixing whitespace. The linter does not seem to like type annotations …
6e9614c 
…for List args
adding missing whitespaces after ,
525652a
adding space before operatiors for linter
c617cef
changing type hint import
b6d5e0f
more whitespace
b9e52b5
adding empty line
8a643e8
removing whitespace from blank linke
3d103b2
adding disable too many lines
e7e9c93
delting draftv2 dir
0ef773c
@karenychen
setup new branch
a52fedb
@karenychen
refactoring
8cc6b05
@karenychen
linked draftv2 create
6ebaf5e
@karenychen
clean up
448188f
@karenychen
rediected stdout and stderr
c911b77
@karenychen karenychen marked this pull request as ready for review April 7, 2022 16:31
@karenychen karenychen changed the title Karenchen/enable draftv2 (WIP) Karenchen/enable draftv2 Apr 7, 2022
@Tatsinnit
Copy link
Copy Markdown
Member

Tatsinnit commented May 21, 2022 •
edited
Loading

💡 Hope this helps: seems like there is a cassette file with this checkin, fyi @FumingZhang in case he notices something and if more help is in need from this feature end, as to why this test is failing. Here are few things I noticed.

cc: @karenychen, fyi: @qpetraroia 🙏☕️

FumingZhang
FumingZhang reviewed May 22, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@FumingZhang FumingZhang left a comment

Choose a reason for hiding this comment

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

Queued a pipeline to test the newly added cases.

Found some problems with concurrent execution. These test cases all try to modify (download)/use the binary in the same location.

Tatsinnit
Tatsinnit reviewed May 22, 2022
View reviewed changes
zhoxing-ms
zhoxing-ms reviewed May 23, 2022
View reviewed changes
@qpetraroia
Copy link
Copy Markdown

qpetraroia commented May 23, 2022

@FumingZhang ready for you to merge when you can 😀

@karenychen karenychen requested a review from FumingZhang May 23, 2022 09:25
@FumingZhang
Copy link
Copy Markdown
Member

FumingZhang commented May 23, 2022

Queued another pipeline to verify test cases. All test cases passed.

FumingZhang
FumingZhang approved these changes May 23, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@FumingZhang FumingZhang left a comment

Choose a reason for hiding this comment

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

LGTM

@zhoxing-ms zhoxing-ms merged commit 229a1cf into Azure:main May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhoxing-ms zhoxing-ms zhoxing-ms approved these changes

@FumingZhang FumingZhang FumingZhang approved these changes

@andyliuliming andyliuliming Awaiting requested review from andyliuliming andyliuliming is a code owner

@wangzelin007 wangzelin007 Awaiting requested review from wangzelin007

+5 more reviewers

@Tatsinnit Tatsinnit Tatsinnit approved these changes

@NickKeller NickKeller NickKeller left review comments

@qpetraroia qpetraroia qpetraroia left review comments

@imiller31 imiller31 imiller31 left review comments

@Marcus-Hines Marcus-Hines Marcus-Hines approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@zhoxing-ms zhoxing-ms

Labels

None yet

Projects

None yet

Milestone

Jun 2022 (2022-07-05)

Development

Successfully merging this pull request may close these issues.

9 participants

@karenychen @yonzhan @zhoxing-ms @Tatsinnit @qpetraroia @FumingZhang @NickKeller @Marcus-Hines @imiller31