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

vm-repair: Bug Fix when HyperV-version Variable is None #1553

Merged
merged 1 commit into from
Apr 26, 2020
Merged

vm-repair: Bug Fix when HyperV-version Variable is None #1553

merged 1 commit into from
Apr 26, 2020

Conversation

swbae31
Copy link
Contributor

@swbae31 swbae31 commented Apr 16, 2020

Quick few liner bug fix when the HyperV-version variable is none

@yonzhan yonzhan requested a review from qwordy April 16, 2020 23:40
@yonzhan yonzhan added this to the S168 milestone Apr 16, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented Apr 16, 2020

add to S168

@swbae31
Copy link
Contributor Author

swbae31 commented Apr 17, 2020

@yonzhan
I got an error from merge validation that
"Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically."

Has some process changed for PRs?

@yonzhan
Copy link
Collaborator

yonzhan commented Apr 17, 2020

Yes, the process is changed to automatically release extensions. @haroldrandom can help with it.

@yonzhan
Copy link
Collaborator

yonzhan commented Apr 17, 2020

You should not change source code and index.json at the same time right now.

@haroldrandom
Copy link
Contributor

haroldrandom commented Apr 17, 2020 •
edited
Loading

src/index.json should't modify both in one PR, we have a release pipeline for publishing your extension automatically.
If you want to use it and hold extension wheel on CLI's official container, just leave src/index.json untouched. After this PR merge, another RP to update src/index.json will be created automatically.
If you still want to hold it in your own container, just fire another PR to update the src/index.json separately.

The detail should be listed in PR template, captured from other PR:

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.

qwordy
qwordy reviewed Apr 17, 2020
View reviewed changes
src/vm-repair/azext_vm_repair/custom.py
Comment on lines +88 to +92
copy_disk_command = 'az disk create -g {g} -n {n} --source {s} --sku {sku} --location {loc} --os-type {os_type} --query id -o tsv' \
.format(g=resource_group_name, n=copy_disk_name, s=target_disk_name, sku=disk_sku, loc=location, os_type=os_type)
# Only add hyperV variable when available
if hyperV_generation:
copy_disk_command += ' --hyper-v-generation {hyperV}'.format(hyperV=hyperV_generation)
Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me.

@swbae31
Copy link
Contributor Author

swbae31 commented Apr 17, 2020

Got it. That's great. Creating the wheel file, uploading it, and making the hash has always been really tedious and made PR changes difficult. This is an awesome change. Thanks.

Then when does the release pipeline happen? And how would I know it has been released with the index.json updated?

@yungezz yungezz assigned qwordy and unassigned yungezz Apr 18, 2020
@yungezz
Copy link
Member

yungezz commented Apr 18, 2020

hi @qwordy could you pls help to review/merge this PR? thanks

@yonzhan yonzhan modified the milestones: S168, S169 Apr 18, 2020
@haroldrandom
Copy link
Contributor

@swbae31

Then when does the release pipeline happen?
It's triggered once PR is merged into master branch but only build and upload only when extension upgrade (detecting via version).

Normally PR developer won't get notifed for the PR to update src/index.json, @fengzhou-msft and I will get notification once because code owner review mechanism. We will assign it to the assignee who responsible for the source PR to trigger this one, then approve it.
You can have a look how the other extension works. :)

@qwordy
Copy link
Member

qwordy commented Apr 24, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@qwordy qwordy merged commit f40506c into Azure:master Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qwordy qwordy qwordy left review comments

@yungezz yungezz yungezz approved these changes

@fengzhou-msft fengzhou-msft Awaiting requested review from fengzhou-msft

@haroldrandom haroldrandom Awaiting requested review from haroldrandom

Assignees

@qwordy qwordy

Labels
None yet
Projects
None yet
Milestone
S169
Development

Successfully merging this pull request may close these issues.
5 participants
@swbae31 @yonzhan @haroldrandom @yungezz @qwordy